1263723 - WebNavigation API should keep track of the user interaction with the urlbar and set transition types and qualifiers accordingly

Status: RESOLVED FIXED

(WebExtensions :: Untriaged, defect)

Description

[Luca Greco [:rpl] [:luca] [:lgreco]]

The WebNavigation API should keep track of how the user is interacting with the urlbar to be able to introduce the following transition types and qualifiers:

- transition qualifiers: from_address_bar
- transition types: typed, generated, keyword, auto_bookmark (when coming from the awesomebar autocompletion)

Comment 1

[Luca Greco [:rpl] [:luca] [:lgreco]]

This change depends on Bug 1256652, which introduces the basic transition types and qualifiers and provides the backbone which this additional Bug can hook its changes on.

Comment 2

[Luca Greco [:rpl] [:luca] [:lgreco]]

Attachment: 3-Bug_1263723____webext__Track_awesomebar_user_interaction_for_webNavigation_transition_types_and_qualifiers_.patch

Comment 3

[Luca Greco [:rpl] [:luca] [:lgreco]]

Attachment: 3-Bug_1263723____webext__Track_awesomebar_user_interaction_for_webNavigation_transition_types_and_qualifiers_.patch

Updated patch (rebased on a recent fx-team tip) with additional test cases (added urlbar's auto_bookmark and keyword test cases).

Comment 4

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 8740391 [details] [diff] [review]
3-Bug_1263723____webext__Track_awesomebar_user_interaction_for_webNavigation_transition_types_and_qualifiers_.patch

patch marked as obsolete (the unpdated patch, rebased and tweaked on the changes on webNavigation already landed, will be pushed to mozreview)

Comment 5

[Luca Greco [:rpl] [:luca] [:lgreco]]

Attachment: MozReview Request: Bug 1263723 - [webext] Track awesomebar user interaction for webNavigation transition types and qualifiers. r=krizsa

- introducing tabTransitionData in the webNavigation internals
- listen for the "autocomplete-did-enter-text" topic notified on the observer service
- add support to from_address_bar transition qualifier and auto_bookmark/keyword/generated transition types

Review commit: https://reviewboard.mozilla.org/r/48971/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/48971/

Comment 6

[Gabor Krizsanits (INACTIVE)]

Comment on attachment 8745294 [details]
MozReview Request: Bug 1263723 - [webext] Track awesomebar user interaction for webNavigation transition types and qualifiers. r=krizsa

https://reviewboard.mozilla.org/r/48971/#review48055

::: browser/components/extensions/test/browser/head.js:141
(Diff revision 1)
> +// Helper used in webNavigation tests related to the urlbar related
> +// (and based on the helpers in 'browser/base/content/test/general/head.js')
> +function waitForCondition(condition, nextTest, errorMsg, retryTimes) {

I'm not super happy about this. I wonder why we have so many mutations of this function already in the tree but creating yet another copy for it just seems wrong.

(http://mxr.mozilla.org/mozilla-central/search?string=function%2BwaitForCondition)

Unfortunately there is not a lot of priority on bug 940882 so I'm not going to hold up your patch, but could you please make this bug depend on that bug with a comment that we had to add yet another copy of this function that will have to be cleaned up later?

::: toolkit/modules/addons/WebNavigation.jsm:84
(Diff revision 1)
>      if (this.listeners.size == 0) {
>        this.uninit();
>      }
>    },
>  
> +  // Support nsIObserver interface to observe the urlbar autocomplete events

Seems like this should be the /* style of comment no?

::: toolkit/modules/addons/WebNavigation.jsm:112
(Diff revision 1)
> +      }
> +
> +      let controller = subject.popup.view.QueryInterface(Ci.nsIAutoCompleteController);
> +      let idx = subject.popup.selectedIndex;
> +      let value = controller.getValueAt(idx);
> +      let action = subject._parseActionUrl(value);

I don't quite get this part, what moz-action stuff do we need here?

::: toolkit/modules/addons/WebNavigation.jsm:130
(Diff revision 1)
> +          // getStyleAt can raise an exception if the search idx doesn't exist
> +          // in the autocompletion popup.

This part worries me. How can getStyleAt throw when getValueAt for the same index has not a few lines ago for the same index? Could you please look into this?

::: toolkit/modules/addons/WebNavigation.jsm:140
(Diff revision 1)
> +      switch (actionType) {
> +        case "bookmark":
> +          tabTransistionData.auto_bookmark = true;
> +          break;
> +        case "typed":
> +          tabTransistionData.typed = true;
> +          break;
> +        case "keyword":
> +          tabTransistionData.keyword = true;
> +          break;
> +        case "searchengine":
> +          tabTransistionData.generated = true;
> +          break;
> +      }

It's not obvious to me why do you need a switch here, can't you just somehow set tabTransitionData directly in place instead of setting actionType?

::: toolkit/modules/addons/WebNavigation.jsm:163
(Diff revision 1)
> +
> +  /**
> +   *  Keep track of a recent user interaction and cache it in a
> +   *  map associated to the current selected tab.
> +   */
> +  pushRecentTabTransitionData(tabTransitionData) {

Push/pop makes me think that we are using a stack here so it's a bit confusing...

::: toolkit/modules/addons/WebNavigation.jsm:183
(Diff revision 1)
> +  /**
> +   *  Retrieve recent data related to a recent user interaction give a
> +   *  given tab's linkedBrowser (only if is is more recent than the
> +   *  RECENT_DATA_THRESHOLD).
> +   */
> +  popRecentTabTransitionData(browser) {

It's not obvious to me that how is it guaranteed that the last transitiondata that we pushed belongs to the onSomethingChange event where we use it, could you explain it to me?

Comment 7

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 8745294 [details]
MozReview Request: Bug 1263723 - [webext] Track awesomebar user interaction for webNavigation transition types and qualifiers. r=krizsa

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/48971/diff/1-2/

Comment 8

[Luca Greco [:rpl] [:luca] [:lgreco]]

(In reply to Luca Greco [:rpl] from comment #7)
> Comment on attachment 8745294 [details]
> MozReview Request: Bug 1263723 - [webext] Track awesomebar user interaction
> for webNavigation transition types and qualifiers. r?krizsa
> 
> Review request updated; see interdiff:
> https://reviewboard.mozilla.org/r/48971/diff/1-2/

This updated version contains the following changes:

- added inline comment on the test helper imported from places (with reference to the related issue) 
- rework url tracking code (e.g. getStyleAt raised exception for out of bound indexes, detect of bookmarks completions, skip the tracking on switchtab moz-action)
- rename the tracking helpers from pushRecentTabTransitionData/popRecentTabTransitionData to setRecentTabTransitionData/getAndForgetRecentTabTransitionData
- added more inline comment to explain the tricky parts
- fixed testcase failure on debug builds

Comment 9

[Luca Greco [:rpl] [:luca] [:lgreco]]

https://reviewboard.mozilla.org/r/48971/#review48055

> I'm not super happy about this. I wonder why we have so many mutations of this function already in the tree but creating yet another copy for it just seems wrong.
> 
> (http://mxr.mozilla.org/mozilla-central/search?string=function%2BwaitForCondition)
> 
> Unfortunately there is not a lot of priority on bug 940882 so I'm not going to hold up your patch, but could you please make this bug depend on that bug with a comment that we had to add yet another copy of this function that will have to be cleaned up later?

Agree, I've added a reference of the above bugzilla issue in the inline comments, and I just added a link and commented about this on Bug 940882.

> Seems like this should be the /* style of comment no?

Done.

> I don't quite get this part, what moz-action stuff do we need here?

This code first try to detect the “keyword” and “searchengine” scenario with the help of the subject._parseAction helper.

Then it is the turn of the "bookmark" scenario, unfortunately I've not found anything better than look into the styles of the autocompletion results (e.g. as currently done in nsBrowserGlue.js, https://dxr.mozilla.org/mozilla-central/source/browser/components/nsBrowserGlue.js#466)

In case the index is not an “out of bounds” or we are unable to detect it, the tracking data defaults to "typed".

NEW: In the last version of this patch I've added a "switch/case" to make explicit how the other  moz-actions are handled, e.g. the switchtab moz-action to be sure that we don't consider it a transition data related to the current tab, because it switches to a different tab and it doesn't generate any navigation event 
(and I'm going to add more test cases for the new scenarios)

> This part worries me. How can getStyleAt throw when getValueAt for the same index has not a few lines ago for the same index? Could you please look into this?

Agree, it worths a deeper look:

- subject._parseAction returns null on bookmark autocomplete results
- getValueAt seems to return an empty string for “out of bounds” indexes (and an additional “WARNING: NS_ENSURE_TRUE(aIndex >= 0 && (uint32_t) aIndex < mRowCount) failed” warning is printed if the code is running on a debug build, but no exception is raised)
- getStyleAt raise an exception for “out of bounds” indexes

In the previous version of this patch both an “out of bounds” index and a bookmark do result in a null action, and so getStyleAt could raise an exception if the branch was entered because of a “out of bounds” index.

In the updated version I’ve reworked the method and now I’m checking that the index is not "out of bounds" before entering the if branch which uses the getStyleAt.

> It's not obvious to me why do you need a switch here, can't you just somehow set tabTransitionData directly in place instead of setting actionType?

In the last pushed version the tabTransitionData are set directly in place as suggested.

> Push/pop makes me think that we are using a stack here so it's a bit confusing...

yeah, agree, I didn't like the push/pop names neither.

In the last version they have been renamed to set/getAndForget.

> It's not obvious to me that how is it guaranteed that the last transitiondata that we pushed belongs to the onSomethingChange event where we use it, could you explain it to me?

This data is going to be used in the "onCommitted" or the "onHistoryStateUpdated/onReferenceFragment" events to better detect the transition types and qualifiers, and so this data is tracked just before the loading event is going to be happen in the target tab, and then once one of the above events is received during the loading (which is supposed to always happen even in case of loading errors) retrieved from the “linkedBrowser -> tabTransitionData” weak map.

I've introduced the "threshold" mostly to prevents us from using this data if the above assumptions break for any reason (and it follows the pattern already used internally by the navigation history internals, https://dxr.mozilla.org/mozilla-central/source/toolkit/components/places/nsNavHistory.cpp#3784, but with a much shorter threshold).

In the ext-webNavigation.js API internals, the final values of the transition type and qualifiers are computed based of the information collected by the webNavigation internals in both the content and the main processes (and in the main process we are going to collect the data related to the user interaction with the tab, in this case through its interaction with the awesome bar).

Comment 10

[Matthew N. [:MattN]]

https://reviewboard.mozilla.org/r/48971/#review48573

::: browser/components/extensions/test/browser/head.js:141
(Diff revision 2)
> +// Helper used in webNavigation tests related to the urlbar related
> +// (and based on the helpers in 'browser/base/content/test/general/head.js')
> +// NOTE: Unfortunately there are already an high number of copies of similar
> +// waitForCondition functions, consolite them into a SimpleTest method
> +// is tracked in Bug 940882.
> +function waitForCondition(condition, nextTest, errorMsg, retryTimes) {
> +  retryTimes = typeof retryTimes !== "undefined" ? retryTimes : 30;
> +  let moveOn;

Please use `BrowserTestUtils.waitForCondition` instead

Comment 11

[Gabor Krizsanits (INACTIVE)]

Comment on attachment 8745294 [details]
MozReview Request: Bug 1263723 - [webext] Track awesomebar user interaction for webNavigation transition types and qualifiers. r=krizsa

https://reviewboard.mozilla.org/r/48971/#review48715

Thanks, this looks great!

::: browser/components/extensions/test/browser/head.js:143
(Diff revisions 1 - 2)
>    return Promise.resolve();
>  }
>  
>  // Helper used in webNavigation tests related to the urlbar related
>  // (and based on the helpers in 'browser/base/content/test/general/head.js')
> +// NOTE: Unfortunately there are already an high number of copies of similar

"an" is not needed

::: toolkit/modules/addons/WebNavigation.jsm:132
(Diff revisions 1 - 2)
> -        case "searchengine":
> +            case "searchengine":
> -          tabTransistionData.generated = true;
> +              tabTransistionData.generated = true;
> -          break;
> +              break;
> +            case "searchsuggestion":
> +              tabTransistionData.generated = true;
> +              break;

instead of break, if the action is similar for multiple cases you can let them fall through:

case: "searchengine":
case: "searchsuggestion":
  tabTransitionData.generated = true;
  break;

Comment 12

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 8745294 [details]
MozReview Request: Bug 1263723 - [webext] Track awesomebar user interaction for webNavigation transition types and qualifiers. r=krizsa

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/48971/diff/2-3/

Comment 13

[Luca Greco [:rpl] [:luca] [:lgreco]]

https://reviewboard.mozilla.org/r/48971/#review48573

> Please use `BrowserTestUtils.waitForCondition` instead

Thanks Matthew!
I've removed the copy of the waitForCondition helper and changed the test case to use BrowserTestUtils.waitForCondition instead.

I'm going to remove the dependency on Bug 940882, luckily we spared ourselves from adding one more variation of that helper.

Comment 14

[Luca Greco [:rpl] [:luca] [:lgreco]]

pushed to try the last version of this patch: 
- https://treeherder.mozilla.org/#/jobs?repo=try&revision=ae93cd61d68c

Comment 15

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/3dc42f7cf449

Comment 16

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/3dc42f7cf449