Closed
Bug 1256652
Opened 9 years ago
Closed 9 years ago
Implements transitionTypes and transitionQualifier attributes in the onCommitted/onHistoryStateUpdated webNavigation events
Categories
(WebExtensions :: Untriaged, defect)
WebExtensions
Untriaged
Tracking
(firefox48 fixed)
Tracking | Status | |
---|---|---|
firefox48 | --- | fixed |
People
(Reporter: rpl, Assigned: rpl)
References
(Blocks 1 open bug)
Details
(Whiteboard: [webNavigation]triaged)
Attachments
(4 files, 1 obsolete file)
The onCommitted and onHistoryStateUpdated webNavigation events should contains the transitionType (which is a string property), and the transitionQualifiers (which is an array of strings, which can be eventually an empty array).
The valid transitionQualifiers values are:
- "from_address_bar"
- "forward_back"
- "server_redirect"
- "client_redirect"
The valid transitionTypes values are:
- "reload"
- "link"
- "form_submit"
- "start_page"
- "manual_subframe"
- "auto_subframe"
- "typed"
- "generated"
- "auto_bookmark"
- "keyword"
- "keyword_generated"
I've collected more details and notes about this properties in the following document (linked to the webNavigation roadmap document):
https://docs.google.com/document/d/1l2RX5PvFTDWhzlzUeKRPces9YZtgHK3QiAZZ41UCvek/edit#
Assignee | ||
Updated•9 years ago
|
Blocks: webext-webnav
Whiteboard: [webNavigation]
Assignee | ||
Updated•9 years ago
|
Summary: Implements transistionTypes and transitionQualifier attributes in the onCommitted/onHistoryStateUpdated webNavigation events → Implements transitionTypes and transitionQualifier attributes in the onCommitted/onHistoryStateUpdated webNavigation events
Assignee | ||
Comment 1•9 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/45031/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/45031/
Assignee | ||
Comment 2•9 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/45143/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/45143/
Assignee | ||
Comment 3•9 years ago
|
||
Comment on attachment 8739075 [details]
MozReview Request: Bug 1256652 - [webext] Initial support of webNavigation transition types and qualifiers. r?krizsa
Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45031/diff/1-2/
Assignee | ||
Updated•9 years ago
|
Assignee: nobody → lgreco
Status: NEW → ASSIGNED
Assignee | ||
Comment 4•9 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/45285/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/45285/
Assignee | ||
Comment 5•9 years ago
|
||
Comment on attachment 8739075 [details]
MozReview Request: Bug 1256652 - [webext] Initial support of webNavigation transition types and qualifiers. r?krizsa
Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45031/diff/2-3/
Assignee | ||
Comment 6•9 years ago
|
||
Comment on attachment 8739467 [details]
MozReview Request: Bug 1256652 - [webext] Add webNavigation form_submit transitions implementation and test case. r=krizsa
Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45285/diff/1-2/
Assignee | ||
Updated•9 years ago
|
Attachment #8739239 -
Attachment is obsolete: true
Assignee | ||
Comment 7•9 years ago
|
||
I've cleaned up the last version of the patches pushed to mozreview related to this Bugzilla issue number,
so that they only include the basic transition types and qualifiers which can be detected using the information
retrieved from the WebNavigationContent.js framescript and their related test cases.
- transition types: reload, link (the default for top level frame transitions), auto_subframe (the default for sub-frame transitions) and form_submit
- transition qualifiers: forward_back, server_redirect
There is still one more transition qualifiers which can be detected in the content process:
- client_redirect (restricted to the refreshers scenario, using an HTTP header or a meta http-equiv tag in the html page)
which needs the fix from Bug 1262794 to be able to detect it with the right timing from a Ci.nsIWebProgressListener2's onRefreshAttempted method.
To implement a greater amount of transition types and qualifiers, the WebNavigation API is going to need more transition data collected in the main process, to be able to keep track of how the user interacted with the tab (e.g. by typing or selecting an item from the awesome bar, by clicking on a bookmark or by explicitly cliking a link into the webpage, which will help to differentiate a manual_subframe from the default auto_subframe transition on sub-frame webNavigation events)
The part which will track the awesome bar (and the other part that needs the help of the NavHistory/Places components) is going to be moved in a separate Bugzilla issue (but I left into this first patches group a backbone and a placeholder for it, mostly because it helps to get a picture on how it will be hooked to the basic implementation introduced by this issue)
Assignee | ||
Updated•9 years ago
|
Attachment #8739075 -
Flags: review?(gkrizsanits)
Assignee | ||
Updated•9 years ago
|
Attachment #8739467 -
Flags: review?(gkrizsanits)
Updated•9 years ago
|
Attachment #8739075 -
Flags: review?(gkrizsanits)
Comment 8•9 years ago
|
||
Comment on attachment 8739075 [details]
MozReview Request: Bug 1256652 - [webext] Initial support of webNavigation transition types and qualifiers. r?krizsa
https://reviewboard.mozilla.org/r/45031/#review42295
::: toolkit/components/extensions/ext-webNavigation.js:35
(Diff revision 3)
> +const tabTransitions = {
> + topFrame: {
> + qualifiers: ["from_address_bar"],
> + types: ["auto_bookmark", "typed", "keyword", "generated", "link"],
> + },
> + subFrame: {
> + types: ["auto_subframe", "manual_subframe"],
> + },
> +};
I don't quite get why is tabTransitions in this patch. I mean if it's only a placeholder (we never use it) then I see no reason to add any of its logic to this patch (this struct and related lines in fillTransitionProperties). It's making reviewing more difficult (both for this and the actual related patch). Would you mind splitting this patch and moving the tabTransations related logic to the bug/patch that deals with that functionality?
::: toolkit/modules/addons/WebNavigationContent.js:137
(Diff revision 3)
> + frameTransitionData.reload = true;
> + }
> +
> + if (request instanceof Ci.nsIChannel) {
> + if (request.loadInfo.redirectChain.length) {
> + frameTransitionData.server_redirect = true;
Is this always the case? I'm thinking about the moz-extensions -> jar type of "redirection"...
In general shouldn't this be handled from onStateChange based on the STATE_REDIRECTING flag?
Updated•9 years ago
|
Attachment #8739467 -
Flags: review?(gkrizsanits) → review+
Comment 9•9 years ago
|
||
Comment on attachment 8739467 [details]
MozReview Request: Bug 1256652 - [webext] Add webNavigation form_submit transitions implementation and test case. r=krizsa
https://reviewboard.mozilla.org/r/45285/#review42313
Assignee | ||
Comment 10•9 years ago
|
||
Comment on attachment 8739075 [details]
MozReview Request: Bug 1256652 - [webext] Initial support of webNavigation transition types and qualifiers. r?krizsa
Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45031/diff/3-4/
Assignee | ||
Comment 11•9 years ago
|
||
Comment on attachment 8739467 [details]
MozReview Request: Bug 1256652 - [webext] Add webNavigation form_submit transitions implementation and test case. r=krizsa
Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45285/diff/2-3/
Assignee | ||
Comment 12•9 years ago
|
||
https://reviewboard.mozilla.org/r/45031/#review42295
> I don't quite get why is tabTransitions in this patch. I mean if it's only a placeholder (we never use it) then I see no reason to add any of its logic to this patch (this struct and related lines in fillTransitionProperties). It's making reviewing more difficult (both for this and the actual related patch). Would you mind splitting this patch and moving the tabTransations related logic to the bug/patch that deals with that functionality?
I agree, this should be in the patch attached Bug 1263723 which makes use of it and can be tested, my apologies.
Moved out from this patch in the last version pushed on mozreview.
> Is this always the case? I'm thinking about the moz-extensions -> jar type of "redirection"...
>
> In general shouldn't this be handled from onStateChange based on the STATE_REDIRECTING flag?
I tested it (and I can introduce a test in a followup, once we have fixed Bug 1246125) and about/moz-extensions/resource/chrome internal url resolving are not detected as server_redirect.
I suppose that I can try to use the STATE_REDIRECTING, but in that case I have to keep track "manually" of the last redirection per DOMWindow, in a way similar to the tracking of the form submit added in this queue of patches.
Assignee | ||
Comment 13•9 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/46467/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/46467/
Attachment #8739075 -
Attachment description: MozReview Request: Bug 1256652 - [webext] Initial support of webNavigation transition types and qualifiers. → MozReview Request: Bug 1256652 - [webext] Initial support of webNavigation transition types and qualifiers. r?krizsa
Attachment #8739467 -
Attachment description: MozReview Request: Bug 1256652 - [webext] Add webNavigation form_submit transitions implementation and test case. → MozReview Request: Bug 1256652 - [webext] Add webNavigation form_submit transitions implementation and test case. r=krizsa
Attachment #8741427 -
Flags: review?(gkrizsanits)
Attachment #8739075 -
Flags: review?(gkrizsanits)
Assignee | ||
Comment 14•9 years ago
|
||
Comment on attachment 8739075 [details]
MozReview Request: Bug 1256652 - [webext] Initial support of webNavigation transition types and qualifiers. r?krizsa
Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45031/diff/4-5/
Assignee | ||
Comment 15•9 years ago
|
||
Comment on attachment 8739467 [details]
MozReview Request: Bug 1256652 - [webext] Add webNavigation form_submit transitions implementation and test case. r=krizsa
Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45285/diff/3-4/
Assignee | ||
Comment 16•9 years ago
|
||
The last push contains:
- rebase on the previous version of the patches on the refactoring that is going to be introduced by Bug 1262794
- added implementation (and test case) of the "client_redirect" transition (currently restricted to the client redirection
related to the meta http-equiv tag and the refresh http header)
Assignee | ||
Comment 17•9 years ago
|
||
Comment on attachment 8741427 [details]
MozReview Request: Bug 1256652 - [webext] Add webNavigation client_redirect transitions implementation and test case. r?krizsa
Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46467/diff/1-2/
Assignee | ||
Comment 18•9 years ago
|
||
Just pushed an update (this time restricted to the last patch in the queue) which adds a test case for the "client_redirect transition, related to the refresh http header scenario.
Unfortunately, by looking at the test cases, we can notice that:
- in the 'meta http-equiv' scenario, the client_redirect transition is detected on the target url onCommitted event
- in the scenario of the refresh http header, the client_redirect transition is detected on the source url onCommitted event.
Assignee | ||
Comment 19•9 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/46737/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/46737/
Attachment #8741765 -
Flags: review?(gkrizsanits)
Assignee | ||
Comment 20•9 years ago
|
||
Comment on attachment 8739075 [details]
MozReview Request: Bug 1256652 - [webext] Initial support of webNavigation transition types and qualifiers. r?krizsa
Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45031/diff/5-6/
Assignee | ||
Comment 21•9 years ago
|
||
Comment on attachment 8739467 [details]
MozReview Request: Bug 1256652 - [webext] Add webNavigation form_submit transitions implementation and test case. r=krizsa
Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45285/diff/4-5/
Assignee | ||
Comment 22•9 years ago
|
||
Comment on attachment 8741427 [details]
MozReview Request: Bug 1256652 - [webext] Add webNavigation client_redirect transitions implementation and test case. r?krizsa
Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46467/diff/2-3/
Assignee | ||
Comment 23•9 years ago
|
||
In the last push:
- fix eslint errors
- add more test cases on sub-frames WebNavigationEvents transition properties
Assignee | ||
Comment 24•9 years ago
|
||
Comment on attachment 8739075 [details]
MozReview Request: Bug 1256652 - [webext] Initial support of webNavigation transition types and qualifiers. r?krizsa
Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45031/diff/6-7/
Assignee | ||
Comment 25•9 years ago
|
||
Comment on attachment 8739467 [details]
MozReview Request: Bug 1256652 - [webext] Add webNavigation form_submit transitions implementation and test case. r=krizsa
Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45285/diff/5-6/
Assignee | ||
Comment 26•9 years ago
|
||
Comment on attachment 8741427 [details]
MozReview Request: Bug 1256652 - [webext] Add webNavigation client_redirect transitions implementation and test case. r?krizsa
Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46467/diff/3-4/
Assignee | ||
Comment 27•9 years ago
|
||
Comment on attachment 8741765 [details]
MozReview Request: Bug 1256652 - [webext] Add more tests on sub-frames WebNavigation transitions properties. r?krizsa
Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46737/diff/1-2/
Updated•9 years ago
|
Whiteboard: [webNavigation] → [webNavigation]triaged
Updated•9 years ago
|
Attachment #8741765 -
Flags: review?(gkrizsanits) → review+
Comment 28•9 years ago
|
||
Comment on attachment 8741765 [details]
MozReview Request: Bug 1256652 - [webext] Add more tests on sub-frames WebNavigation transitions properties. r?krizsa
https://reviewboard.mozilla.org/r/46737/#review44133
Comment 29•9 years ago
|
||
Comment on attachment 8739075 [details]
MozReview Request: Bug 1256652 - [webext] Initial support of webNavigation transition types and qualifiers. r?krizsa
https://reviewboard.mozilla.org/r/45031/#review44135
Attachment #8739075 -
Flags: review?(gkrizsanits) → review+
Comment 30•9 years ago
|
||
Comment on attachment 8741427 [details]
MozReview Request: Bug 1256652 - [webext] Add webNavigation client_redirect transitions implementation and test case. r?krizsa
https://reviewboard.mozilla.org/r/46467/#review44137
Attachment #8741427 -
Flags: review?(gkrizsanits) → review+
Assignee | ||
Updated•9 years ago
|
Iteration: --- → 48.3 - Apr 25
Keywords: checkin-needed
Comment 31•9 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/79a7dc93f547
https://hg.mozilla.org/integration/fx-team/rev/fe58e0490f35
https://hg.mozilla.org/integration/fx-team/rev/962cb71c40b1
https://hg.mozilla.org/integration/fx-team/rev/78d531b24e53
Keywords: checkin-needed
Comment 32•9 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/79a7dc93f547
https://hg.mozilla.org/mozilla-central/rev/fe58e0490f35
https://hg.mozilla.org/mozilla-central/rev/962cb71c40b1
https://hg.mozilla.org/mozilla-central/rev/78d531b24e53
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox48:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Updated•6 years ago
|
Product: Toolkit → WebExtensions
You need to log in
before you can comment on or make changes to this bug.
Description
•