Modify

Opened 12 years ago

Closed 12 years ago

Last modified 12 years ago

#10748 closed enhancement (fixed)

For optimal performance req.chrome should be evaluated only if necessary.

Reported by: Olemis Lang Owned by: Olemis Lang
Priority: normal Component: ThemeEnginePlugin
Severity: minor Keywords: chrome css script
Cc: peter@…, Gary Martin Trac Release: 1.0

Description

When the plugin is installed and active it's post_process_request() will be invoked for every request. AFAICT there is nothing in that code that would differentiate requests (API vs UI requests) so add_stylesheet() will be called for all of them which then calls add_link() that evaluates req.chrome.

mentioned in BH:comment:3:ticket:330

Attachments (0)

Change History (8)

comment:1 Changed 12 years ago by Olemis Lang

Status: newassigned

comment:4 Changed 12 years ago by Olemis Lang

Type: defectenhancement

(In [12508]) ThemeEnginePlugin : Do not access req.chrome while filtering requests if template = data = None ( refs #10748 )

comment:5 Changed 12 years ago by Ryan J Ollos

Cc: Gary Martin added

I cleaned up the reference in your commit message :)

One minor, almost irrelevant comment that I'd like to get your thoughts on. PEP8 states: Comparisons to singletons like None should always be done with is or is not, never the equality operators.

So I was thinking that would suggest the following change:

if (template, data) != (None, None):

->

if (template, data) is not (None, None)

(I wish individual bullet points on PEP8 had anchors so that we could link to them individually. The section headings have anchors, but even for those the browser doesn't seem to jump to the right location [at least in Chrome 23, try: http://www.python.org/dev/peps/pep-0008/#id37, it should go Programming Recommendations section]).

comment:6 in reply to:  5 Changed 12 years ago by Olemis Lang

Replying to rjollos:

[...]

So I was thinking that would suggest the following change:

if (template, data) != (None, None):

->

if (template, data) is not (None, None)

Bad idea , mainly because of this

>>> (None, None) is (None, None)
False

[...]

comment:7 Changed 12 years ago by Ryan J Ollos

Ah, right, because in the latter case it will be testing if the two references are the same. Thanks for the clarification.

comment:6 Changed 12 years ago by Olemis Lang

(In [12534]) ThemeEnginePlugin: [regression] Theme struct will be None if no theme configured or set to 'default' - refs #10748 fixes #10800

comment:7 Changed 12 years ago by Olemis Lang

Resolution: fixed
Status: assignedclosed

This is done. Please reopen if anything weird is detected .

comment:8 Changed 12 years ago by Olemis Lang

(In [12540]) ThemeEnginePlugin: Let theme post-process requests while rendering error pages - after [12508] refs #10748 fixes #10809

Modify Ticket

Change Properties
Set your email in Preferences
Action
as closed The owner will remain Olemis Lang.
The resolution will be deleted. Next status will be 'reopened'.

Add Comment


E-mail address and name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.