Closed Bug 1256652 Opened 4 years ago Closed 3 years ago

Implements transitionTypes and transitionQualifier attributes in the onCommitted/onHistoryStateUpdated webNavigation events

Categories

(WebExtensions :: Untriaged, defect)

defect
Not set

Tracking

(firefox48 fixed)

RESOLVED FIXED
mozilla48
Iteration:
48.3 - Apr 25
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#
Whiteboard: [webNavigation]
Summary: Implements transistionTypes and transitionQualifier attributes in the onCommitted/onHistoryStateUpdated webNavigation events → Implements transitionTypes and transitionQualifier attributes in the onCommitted/onHistoryStateUpdated webNavigation events
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: nobody → lgreco
Status: NEW → ASSIGNED
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/
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/
Attachment #8739239 - Attachment is obsolete: true
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)
Attachment #8739075 - Flags: review?(gkrizsanits)
Attachment #8739467 - Flags: review?(gkrizsanits)
Blocks: 1263723
Attachment #8739075 - Flags: review?(gkrizsanits)
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?
Attachment #8739467 - Flags: review?(gkrizsanits) → review+
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
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/
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/
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.
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)
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/
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/
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)
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/
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.
Blocks: 1264936
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/
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/
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/
In the last push:

- fix eslint errors
- add more test cases on sub-frames WebNavigationEvents transition properties
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/
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/
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/
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/
Whiteboard: [webNavigation] → [webNavigation]triaged
Attachment #8741765 - Flags: review?(gkrizsanits) → review+
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 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 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+
Iteration: --- → 48.3 - Apr 25
Keywords: checkin-needed
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.