Last Comment Bug 768950 - Port |Bug 763171 - Display received tabs|
: Port |Bug 763171 - Display received tabs|
Status: RESOLVED FIXED
:
Product: SeaMonkey
Classification: Client Software
Component: Sync UI (show other bugs)
: Trunk
: All All
: -- normal (vote)
: seamonkey2.13
Assigned To: Jens Hatlak (:InvisibleSmiley)
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-06-27 10:07 PDT by Jens Hatlak (:InvisibleSmiley)
Modified: 2012-07-02 08:36 PDT (History)
1 user (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
patch (2.18 KB, patch)
2012-06-27 10:07 PDT, Jens Hatlak (:InvisibleSmiley)
neil: review-
Details | Diff | Review
test case (911 bytes, text/plain)
2012-06-27 10:09 PDT, Jens Hatlak (:InvisibleSmiley)
no flags Details
patch v2 [Checkin: Comment 9] (4.71 KB, patch)
2012-06-29 14:04 PDT, Jens Hatlak (:InvisibleSmiley)
neil: review+
gps: feedback-
Details | Diff | Review

Description Jens Hatlak (:InvisibleSmiley) 2012-06-27 10:07:24 PDT
Created attachment 637171 [details] [diff] [review]
patch

Like Firefox, we should support the displayURI Sync command for receiving individual links to be opened in a tab.

NB: The issuing side is currently only supported by mobile Firefox. Will attach a test case (e.g. to be executed in Execute JS from xsidebar.mozdev.org) in a minute.
Comment 1 Jens Hatlak (:InvisibleSmiley) 2012-06-27 10:09:06 PDT
Created attachment 637173 [details]
test case

The attached test case is based on test_clients_engine.js. It outputs to the Error Console for verification purposes only.
Comment 2 neil@parkwaycc.co.uk 2012-06-28 16:06:24 PDT
Comment on attachment 637171 [details] [diff] [review]
patch

This fires once per browser window, which would be bad any time when we didn't have exactly one browser window open. Sadly I can't find a good place to put some code to handle the case when we have no browser windows open. (The best I can find would be similar to the way the tasksOverlay code passes a dummy command line to nsBrowserContentHandler, but in this case you would have to use a command which then passes the desired URL to handURIToExistingBrowser.)
Comment 3 Gregory Szorc [:gps] 2012-06-28 16:10:07 PDT
There is a new patch on the original bug that fixes the multi-window issue.
Comment 4 Jens Hatlak (:InvisibleSmiley) 2012-06-29 14:04:44 PDT
Created attachment 638006 [details] [diff] [review]
patch v2 [Checkin: Comment 9]

Thanks Neil for catching the multi/non-window cases. The attached should take care of both issues.

Gavin raised the question on the base bug whether the new tab should be focused. After some thinking my answer is Yes: This command is currently only used for actively sending a tab from a mobile device to a computer, and I think that in that particular case the user's will and expectancy is that the tab is displayed on the computer and raised to the foreground.
Comment 5 Gregory Szorc [:gps] 2012-06-29 14:07:58 PDT
(In reply to Jens Hatlak (:InvisibleSmiley) from comment #4)
> Gavin raised the question on the base bug whether the new tab should be
> focused. After some thinking my answer is Yes: This command is currently
> only used for actively sending a tab from a mobile device to a computer, and
> I think that in that particular case the user's will and expectancy is that
> the tab is displayed on the computer and raised to the foreground.

UX explicitly requested that the tab be opened in the background. You may also wish to carry forward the comment in my patch that states why.
Comment 6 neil@parkwaycc.co.uk 2012-06-30 15:53:41 PDT
Comment on attachment 638006 [details] [diff] [review]
patch v2 [Checkin: Comment 9]

>+      if (mostRecentBrowserWindow) {
>+        mostRecentBrowserWindow.getBrowser().addTab(url, { focusNewTab: true });
[I wonder whether we should raise the window too, i.e. mostRecentBrowserWindow.content.focus();]

>+        Services.ww.openWindow(null, "chrome://navigator/content/navigator.xul", "_blank",
Should check the browser.chromeURL pref; all of our other callers do.
(Followup bug if you prefer.)
Comment 7 Gregory Szorc [:gps] 2012-06-30 16:02:31 PDT
Comment on attachment 638006 [details] [diff] [review]
patch v2 [Checkin: Comment 9]

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

Per UX, the tab is supposed to open in the background. This does not violate the principle of least surprise. Having the browser do unexpected things automatically is not good.

That being said, this whole feature is currently a minor exception because Fennec allows sending tabs to clients that don't support displaying tabs because of a limitation in Sync. UX has stated that having the browser do something somewhat unexpected in this case is a lesser evil than having the tab disappear into the ether.

We (Sync) are working on a better long-term solution. See also bug 769744.

See https://hg.mozilla.org/services/services-central/rev/9d0bf5dddfc4 for UX-approved implementation, consistent with Firefox.
Comment 8 neil@parkwaycc.co.uk 2012-07-02 07:34:30 PDT
(In reply to Gregory Szorc from comment #7)
> Per UX, the tab is supposed to open in the background.
It is legitimate for SeaMonkey to have a different UX to Firefox. For instance we could follow up by adding a preference to choose whether to focus or not.

> Having the browser do unexpected things automatically is not good.
Well, that depends on how you define "automatic".
Comment 9 Jens Hatlak (:InvisibleSmiley) 2012-07-02 08:36:22 PDT
Comment on attachment 638006 [details] [diff] [review]
patch v2 [Checkin: Comment 9]

http://hg.mozilla.org/comm-central/rev/4e1ef6937c5c
(with comment 6 addressed)

Note You need to log in before you can comment on or make changes to this bug.