Modify

Opened 19 months ago

Closed 19 months ago

Last modified 19 months ago

#10748 closed enhancement (fixed)

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

Reported by: olemis Owned by: olemis
Priority: normal Component: ThemeEnginePlugin
Severity: minor Keywords: chrome css script
Cc: peter@…, gmartin 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 19 months ago by olemis

  • Status changed from new to assigned

comment:4 Changed 19 months ago by olemis

  • Type changed from defect to enhancement

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

comment:5 follow-up: Changed 19 months ago by rjollos

  • Cc gmartin 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 19 months ago by olemis

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 19 months ago by rjollos

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 19 months ago by olemis

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

comment:7 Changed 19 months ago by olemis

  • Resolution set to fixed
  • Status changed from assigned to closed

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

comment:8 Changed 19 months ago by olemis

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

Add Comment

Modify Ticket

Action
as closed .
as The resolution will be set. Next status will be 'closed'.
to The owner will be changed from olemis. Next status will be 'closed'.
The resolution will be deleted. Next status will be 'reopened'.
Author


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

 
Note: See TracTickets for help on using tickets.