Closed
Bug 642447
Opened 13 years ago
Closed 13 years ago
change default contentScriptWhen to new 'end' option that fires on 'load' event
Categories
(Add-on SDK Graveyard :: General, defect, P1)
Add-on SDK Graveyard
General
Tracking
(Not tracked)
RESOLVED
FIXED
1.0b5
People
(Reporter: ochameau, Assigned: ochameau)
References
Details
(Keywords: APIchange, dev-doc-needed, relnote)
Attachments
(2 files, 4 obsolete files)
41.64 KB,
patch
|
myk
:
review-
|
Details | Diff | Splinter Review |
41.29 KB,
patch
|
myk
:
review+
|
Details | Diff | Splinter Review |
I think that in page-mod usage this is the common value people would expect. As main page-mod is about playing with DOM nodes, having the default value set to start force them to add listener for load event ... or for beginner just do not understand why it is failing. And as side effect it will lower effects of bug 641457. Finally, I think that we may have some issues in providing 'start' event with E10s context. As chrome listener that receive 'document-element-inserted' won't necessary be able to communicate with content document and apply contentScript quick enough.
Attachment #519903 -
Flags: review?(myk)
Comment 1•13 years ago
|
||
+1 from me on this change. There's very few instances where I don't use ready. Regarding e10s, we can file a bug and have add a new notification added if we need it.
Comment 2•13 years ago
|
||
I agree, this makes sense, even though it is a breaking change in the beta phase (the first such change, to our credit), so we must communicate it thoroughly in the docs, release notes, release announcement, and a post to the discussion group. And given that it's a breaking change, it's worth digging in further to make sure it's exactly the right one. So I went looking to see what other addon platforms do, and both Chrome's and Safari's default to or recommend loading content scripts around the time the page is fully loaded (i.e. the load event fires), because doing so prioritizes page load over content script execution, and users are first and foremost interested in accessing the pages they load and only secondarily interested in having them modified by content scripts. For Chrome Extensions, the default value of run_at is "idle", which loads scripts "between [DOMContentLoaded] and immediately after the window.onload event fires." <http://code.google.com/chrome/extensions/content_scripts.html> For Safari Extensions, "an End Script executes when the DOM is fully loaded—at the time the onload attribute of a body element would normally fire. Most scripts should be injected as End Scripts." <http://developer.apple.com/library/safari/#documentation/Tools/Conceptual/SafariExtensionGuide/InjectingScripts/InjectingScripts.html#//apple_ref/doc/uid/TP40009977-CH6-SW1>. Chrome's "idle" seems funky. It prioritizes page load performance but isn't clearly distinct from DOMContentLoaded and doesn't provide addon developers with a known page state at the time of the event. Safari's "end" does do that, however, while similarly prioritizing page load performance. It's also compatible with "idle", given that "end" is one of the times at which "idle" may occur. We should emulate Safari's "end" and default to executing content scripts when the load event fires via an "end" value for contentScriptWhen that becomes the new default value for that property. (In reply to comment #0) > And as side effect it will lower effects of bug 641457. Yup, but let's make sure we still push for the platform bug to get fixed, as we still need it in order for "start" to work correctly. > Finally, I think that we may have some issues in providing 'start' event with > E10s context. As chrome listener that receive 'document-element-inserted' won't > necessary be able to communicate with content document and apply contentScript > quick enough. Yes, this may be involved, f.e. the module might have to hook into content process setup with some code that blocks on document-element-inserted until the content script can be loaded. But as Dietrich notes, we can and should tackle this separately. It isn't a reason to give up on the very handy "start". Alex: can you update your patch to default the property to a new "end" value that executes content scripts when the page's "load" event fires?
Assignee: nobody → poirot.alex
Keywords: dev-doc-needed
Summary: Change default contentScriptWhen to 'ready' → change default contentScriptWhen to new 'end' option that fires on 'load' event
Updated•13 years ago
|
Attachment #519903 -
Flags: review?(myk)
Assignee | ||
Comment 3•13 years ago
|
||
I've translated all "ready" to "end" everywhere: code and docs. I've updated the contentScriptWhen documentation, but I think broader modifications are needed, like: - Simplify, remove or change first examples in page-mod: https://github.com/mozilla/addon-sdk/blob/master/packages/addon-kit/docs/page-mod.md - May be remove all contentScriptWhen by assuming default value is good, and only mention it in contentScriptWhen documentation that there is something to do with contentScriptWhen for special start case. And of course having an explicit start usage in dev-guide. https://github.com/mozilla/addon-sdk/blob/master/static-files/md/dev-guide/addon-development/web-content.md Do we create another bug item for doc ?
Attachment #519903 -
Attachment is obsolete: true
Attachment #522516 -
Flags: review?(myk)
Assignee | ||
Comment 4•13 years ago
|
||
Oops, forgot a debug console.log call!
Attachment #522516 -
Attachment is obsolete: true
Attachment #522516 -
Flags: review?(myk)
Attachment #522518 -
Flags: review?(myk)
Comment 5•13 years ago
|
||
> Do we create another bug item for doc ? -> Bug 645885
Updated•13 years ago
|
Priority: -- → P1
Target Milestone: --- → 1.0b5
Comment 6•13 years ago
|
||
Comment on attachment 522518 [details] [diff] [review] contentScriptWhen defaults to "end" Erm, this adds "end" and makes it the default but makes it fire on DOMContentLoaded and makes "ready" a synonym for it; whereas what we actually want here is to add "end", make it the default, make it fire on page load, and keep "ready" as the value that fires on DOMContentLoaded. Thus the three possible values of contentScriptWhen should become: start: fire on content-document-element-created ready: fire on DOMContentLoaded end: fire on "load" (default)
Attachment #522518 -
Flags: review?(myk) → review-
Assignee | ||
Comment 7•13 years ago
|
||
Here is the new one. Aditionally I fixed some bug on my previous implementation. About documentations, I removed a lot of contentScriptWhen: "ready" except some in pageMod. I think that default value will work for most cases and so it is better to simplify most of our examples and let developer discover this value in a particular pageMod documentation that may need some improvement.
Attachment #522518 -
Attachment is obsolete: true
Attachment #525144 -
Flags: review?(myk)
Comment 8•13 years ago
|
||
Comment on attachment 525144 [details] [diff] [review] contentScriptWhen with start, ready or end. defaults to "end" >diff --git a/packages/addon-kit/data/test-page-worker.js b/packages/addon-kit/data/test-page-worker.js >+postMessage(["assertEqual", document.title, "Page Worker test", >+ "Correct page title accessed directly"]); Nit: here and elsewhere, indent arguments like so: postMessage(["assertEqual", document.title, "Page Worker test", "Correct page title accessed directly"]); >diff --git a/packages/addon-kit/docs/page-mod.md b/packages/addon-kit/docs/page-mod.md >-If you specify a value of "ready" for `contentScriptWhen` then the content >+If you specify a value of "end" for `contentScriptWhen` then the content > script can interact with the DOM itself: Actually both "ready" and "when" allow you to interact with the DOM (technically "start" does as well, although most of the DOM isn't loaded yet), so this should say: If you specify a value of "ready" or "end" for `contentScriptWhen`, then the content script can interact with the DOM itself: >+ ready (ie like DOMContentLoaded event), and "start", which loads them as >+ soon as the window object for the page has been created. Nit: ie -> i.e. Nit: "start" should be described as "loads them once the `window` object for the page has been created, but before any scripts specified by the page have been loaded." >diff --git a/packages/addon-kit/lib/page-mod.js b/packages/addon-kit/lib/page-mod.js >+ // bug 641457: document-element-inserted is fired multiple times. >+ // Until this bug is fixed we prevent multiple workers for being created >+ _loadingWindows: [], Nit: for -> from But this is actually platform bug 642447, which was fixed in Firefox Macaw <https://wiki.mozilla.org/Releases/Firefox_4/Macaw>, a.k.a. Firefox 4.0.1, scheduled for release on April 26. So we should omit this workaround. Use the RCs <https://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/4.0.1-candidates/build1/> to test the feature without the workaround. >+ if (event.target.defaultView != window) return; Nit: here and elsewhere, put such blocks on the next line, i.e.: if (event.target.defaultView != window) return; >diff --git a/packages/addon-kit/tests/test-page-mod.js b/packages/addon-kit/tests/test-page-mod.js >+ // As testPageMod callback with asserts is called on 'end' >+ // We want the pagemod to operate before this event >+ contentScriptWhen: 'start', Nit: this took me a bit to parse. I would change it to read: // The testPageMod callback with test assertions is called on 'end', // and we want this page mod to be attached before it gets called, // so we attach it on 'start'. Otherwise, this looks good!
Attachment #525144 -
Flags: review?(myk) → review-
Assignee | ||
Comment 9•13 years ago
|
||
Here is a first patch that still contains the platform fix, but all review comment are addressed. I'd like to let it for some time and remove it later when nobody will still use FF 4.0.
Attachment #525144 -
Attachment is obsolete: true
Attachment #527254 -
Flags: review?(myk)
Assignee | ||
Comment 10•13 years ago
|
||
In case you think that this fix really doesn't worth being commited. Thanks for all these quick reviews, it'll make 1.0b5 version a great stabilization'n tweak release!
Attachment #527257 -
Flags: review?(myk)
Assignee | ||
Comment 11•13 years ago
|
||
Btw, I tested without the fix, and it's working fine on builds you mentioned: https://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/4.0.1-candidates/build1/
Comment 12•13 years ago
|
||
Comment on attachment 527254 [details] [diff] [review] Same patch with platform bug fix Firefox 4.0.1 is shipping either this Thursday or next Tuesday <https://wiki.mozilla.org/WeeklyUpdates/2011-04-25#Firefox_4.2F3.6.2F3.5>, before SDK 1.0b5 and well before SDK 1.0 final, so let's leave out the platform fix.
Attachment #527254 -
Flags: review?(myk) → review-
Comment 13•13 years ago
|
||
Comment on attachment 527257 [details] [diff] [review] Same patch without platform fix Looks good, works well, r=myk!
Attachment #527257 -
Flags: review?(myk) → review+
Assignee | ||
Comment 14•13 years ago
|
||
Landed! https://github.com/mozilla/addon-sdk/commit/22d182e50912c0a8c8b21b183cdeaa9925df68be
Assignee | ||
Comment 15•13 years ago
|
||
We have to warn users to use very last Firefox 4.0.1 version. It took me ages to figure out that annotator example was broken because of the platform bug. It causes page-mod content script to be executed multiple times.
Keywords: relnote
Updated•13 years ago
|
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•