1248251 - We should not send the referrer when a link is open in a different container.

Status: RESOLVED FIXED

(Firefox :: Menus, defect)

Description

[Andrea Marchesini [:baku]]

Attachment: aa.patch

Comment 1

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 8719282 [details] [diff] [review]
aa.patch

Review of attachment 8719282 [details] [diff] [review]:
-----------------------------------------------------------------

This is good but I think it needs a test.

Comment 2

[Andrea Marchesini [:baku]]

Can you give me a similar existing test?

Comment 3

[Andrea Marchesini [:baku]]

Attachment: test

Here a test.

Comment 4

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 8719424 [details] [diff] [review]
test

Review of attachment 8719424 [details] [diff] [review]:
-----------------------------------------------------------------

I think instead of extending checkReferrerAndStartNextTest() it would be better to override |getReferrerTest| (simply by defining it in the test file) and provide your own test cases, even if they end up the same with just the expected |result| changed.

::: browser/base/content/browser-context.inc
@@ +65,5 @@
>              accesskey="&openLinkCmdInContainerTab.accesskey;"
>              hidden="true">
>          <menupopup oncommand="gContextMenu.openLinkInTab(event);">
>            <menuitem label="&userContextPersonal.label;"
> +                    id="context-openlinkinusercontext-personal"

Shouldn't those contexts be flexible? It seems rather wrong to give them specific IDs. Maybe just pick the first in the list?

::: browser/base/content/test/referrer/browser_referrer_open_link_in_container_tab.js
@@ +1,2 @@
> +// Tests referrer on context menu navigation - open link in new tab.
> +// Selects "open link in new tab" from the context menu.

This comment should be updated.

::: browser/base/content/test/referrer/head.js
@@ +212,5 @@
>   * Checks the result of the referrer test, and moves on to the next test.
>   * @param aTestNumber The test number - 0, 1, 2, ...
>   * @param aNewWindow The new window where the referrer target opened, or null.
>   * @param aNewTab The new tab where the referrer target opened, or null.
>   * @param aStartTestCase The callback to start the next test, aTestNumber + 1.

Please document the new parameter.

@@ +215,5 @@
>   * @param aNewTab The new tab where the referrer target opened, or null.
>   * @param aStartTestCase The callback to start the next test, aTestNumber + 1.
>   */
>  function checkReferrerAndStartNextTest(aTestNumber, aNewWindow, aNewTab,
> +                                       aStartTestCase, resultOverwritten = undefined) {

Isn't that just undefined if not given? The default value seems unnecessary. Also wouldn't |resultOverride| be a better name? It currently sounds like I'd have to pass a boolean instead of a value to overwrite the result with.

@@ +222,5 @@
>      let test = getReferrerTest(aTestNumber);
>      let desc = getReferrerTestDescription(aTestNumber);
> +
> +    let testResult = resultOverwritten != undefined ? resultOverwritten : test.result;
> +    is(result, testResult, desc);

We can reduce this to:

is(result, resultOverride || test.result, desc);

Comment 5

[Andrea Marchesini [:baku]]

Attachment: test

Comment 6

[Andrea Marchesini [:baku]]

Attachment: aa.patch

Comment 7

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/10ea38335439
https://hg.mozilla.org/integration/mozilla-inbound/rev/b00f72a9e515

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/10ea38335439
https://hg.mozilla.org/mozilla-central/rev/b00f72a9e515