Closed Bug 1144015 Opened 5 years ago Closed 5 years ago

(Browser API) Fire mozbrowseropentab on ctrl-click and middle click

Categories

(Core :: DOM: Core & HTML, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla40
Tracking Status
firefox40 --- fixed

People

(Reporter: paul, Assigned: paul)

References

Details

Attachments

(1 file, 4 obsolete files)

No description provided.
Attached patch v1 (obsolete) — Splinter Review
Assignee: nobody → paul
Status: NEW → ASSIGNED
Attachment #8578468 - Flags: review?(justin.lebar+bug)
Blocks: graphene
Duplicate of this bug: 1124111
Attachment #8578468 - Flags: review?(justin.lebar+bug) → review?(kchen)
Comment on attachment 8578468 [details] [diff] [review]
v1

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

::: dom/browser-element/BrowserElementChildPreload.js
@@ +593,5 @@
> +      // Open in a new tab if middle click or ctrl/cmd-click.
> +      if ((Services.appinfo.OS == 'Darwin' && e.metaKey) ||
> +          (Services.appinfo.OS != 'Darwin' && e.ctrlKey) ||
> +           e.button == 1) {
> +        sendAsyncMsg('openwindow', {

Use a new event name? The original mozbrowseropenwindow has a different meaning.
Attachment #8578468 - Flags: review?(kchen) → feedback+
And test case please :)
Attached patch v2 (obsolete) — Splinter Review
Attachment #8578468 - Attachment is obsolete: true
Attachment #8584431 - Flags: review?(kchen)
Comment on attachment 8584431 [details] [diff] [review]
v2

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

::: dom/browser-element/mochitest/mochitest.ini
@@ +122,5 @@
>  # process.  Default is OOP.
>  [test_browserElement_NoAttr.html]
>  [test_browserElement_NoPref.html]
>  [test_browserElement_NoPermission.html]
> +[test_browserElement_OpenTab.html]

So this test case works in the inproc variant. Does it work out-of-process? Create _inproc_ and _oop_ tests accordingly.

::: dom/browser-element/mochitest/test_browserElement_OpenTab.html
@@ +1,4 @@
> +<!DOCTYPE HTML>
> +<html>
> +<!--
> +https://bugzilla.mozilla.org/show_bug.cgi?id=741587

Wrong link
Attachment #8584431 - Flags: review?(kchen) → feedback+
Test times out on emulator. But it doesn't make sense to test this on touch-only platforms.
Summary: (Browser API) Fire mozbrowseropenwindow on ctrl-click and middle click → (Browser API) Fire mozbrowseropentab on ctrl-click and middle click
Attachment #8586560 - Flags: review?(kchen)
Comment on attachment 8586560 [details] [diff] [review]
v2.2

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

Looks good. One last thing: put test_browserElement_inproc_OpenTab.html in mochitest.ini, test_browserElement_oop_OpenTab.html in mochitest-oop.ini
Attachment #8586560 - Flags: review?(kchen) → review+
(In reply to Kan-Ru Chen [:kanru] from comment #10)
> Comment on attachment 8586560 [details] [diff] [review]
> v2.2
> 
> Review of attachment 8586560 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good. One last thing: put test_browserElement_inproc_OpenTab.html in
> mochitest.ini, test_browserElement_oop_OpenTab.html in mochitest-oop.ini

inproc won't work as it's intercepted by Firefox tabs in browser mochitest.
Keywords: checkin-needed
https://hg.mozilla.org/integration/b2g-inbound/rev/65ff49ab095d

Going off the assumption that those m-dt failures on the Try push were from a bad parent rev and not this patch. We'll see :)
Flags: in-testsuite+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/65ff49ab095d
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
You need to log in before you can comment on or make changes to this bug.