617804 - Add platform support for document status

Status: RESOLVED FIXED

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

Description

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Currently, TabChild::SetStatus() and SetTitle() are being called completely legitimately, but their implementation is NS_NOTREACHED(), which is untrue and just results in assertion spam.

We should decide on one of two options
 (1) chrome process is responsible for managing these, which means chrome-privileged script in content processes is responsible for forwarding these as desired.  Fennec must be doing something like this currently
 (2) the platform is responsible for forwarding these

It would be nice to find a solution that make both ff.next and fennec happy.

Comment 1

[Matt Brubeck (:mbrubeck)]

We currently handle this in our <browser remote> XBL binding.  The binding is designed to be reusable outside of Fennec, so one option is to use the same binding in Fx.next.

Comment 2

[:Felipe Gomes (needinfo for replies!)]

You do this through the DOMTitleChanged message, right?

I'm curious on how TabChild::SetTitle ends up not called on Fennec. Or is it called and the NS_NOTREACHED is just being ignored?

Comment 3

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

TabChild::SetTitle is being called and ignored.

Comment 4

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: part 1: Change bogus NOTREACHED() assertions

This patch is just to unblock bug 615386.

Comment 6

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Oops, missed one.  Silencing these bogus notreacheds blocks turning on reftest-ipc.

Comment 7

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

http://hg.mozilla.org/mozilla-central/rev/53958cd8b528

Leaving open for potential platform support.

Comment 8

[:Felipe Gomes (needinfo for replies!)]

To support the document title in FF there's the typical question of implementing this in the platform or JS.

Currently, the JS UI code listens to the DOMTitleChanged event, and then retrieves the title through browser.contentTitle (which won't work with e10s).

With e10s, the content calls TabChild::SetTitle, which we can use to generate the DOMTitleChanged in the parent process. However the title is not included in this generic event, so the chrome would have to cross the process-boundary again to retrieve the title string.

Some possibilities:
 - implement it as descrived above

 - redefine DOMTitleChanged as a DOMMessageEvent, include the tab title in the event.data field, change JS listener to use that instead (avoid crossing the process again)

 - don't bother with TabChild::SetTitle at all, send title changes through messagemanager with the title in the json object. (that's how Fennec currently does it, and Mehdi wrote a patch with this approach for FF)


cjones, bsmedberg: thoughts?

Comment 9

[Mark Finkle (:mfinkle) (use needinfo?)]

My own input on this is: If you can't make the event be _exactly_ like the current DOM event in state and behavior, then don't even try. For JS-based fornt-ends, using the messagemanager will be your best option. Also know that the JS message manager will give you opportunities to reduce overall message IPC traffic by consoldiating only the state you really need for the application.

Lastly, browser XBL binding already has a "contentTitle" property that Fennec keeps updated using the DOMTitleChanged message (not event), so subsequent "hey, what's the current title" does not require storing state as needed, or sending IPC messages for each access.

Aside, we need to get the Fennec IPC-friendly browser XBL bindings into mozilla-central.

Comment 10

[Benjamin Smedberg]

I believe we should use the messagemanager approach and make TabChild::SetTitle a no-op.

Comment 11

[Tom S [:evilpie]]

Attachment: SetStatusWithContext

This implements StatusWithContext by using ipdl. This seems strictly easier then JS way suggested previously. This makes link tooltips in the UI work, but they sadly they never disappear. I think this might be a problem with the mousemove event in LinkTargetDisplay in browser.js. I would appreciate some help from the fx-team with this.

Comment 12

[:Felipe Gomes (needinfo for replies!)]

The counterpart of nsDocShell::onOverLink is nsDocShell::onLeaveLink, which calls SetStatus instead of SetStatusWithContext.  So if you also implement TabChild::SetStatus, and presumably it will call SendSetStatus with an empty string, then the disappearing should work too

Comment 13

[Tom S [:evilpie]]

Attachment: v2

Thanks, this was a good hint.

Comment 14

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 781160 [details] [diff] [review]
v2

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

::: dom/ipc/PBrowser.ipdl
@@ +189,5 @@
>  
>      SetCursor(uint32_t value);
>      SetBackgroundColor(nscolor color);
>  
> +    SetStatus(uint32_t type, nsString status);

Lots of the messages in this file didn't get documentation but let's add it whenever possible. A brief comment saying what this is used for (over link support) is enough

::: dom/ipc/TabParent.cpp
@@ +757,5 @@
> +TabParent::RecvSetStatus(const uint32_t& aType, const nsString& aStatus)
> +{
> +  nsCOMPtr<nsIContent> frame = do_QueryInterface(mFrameElement);
> +  if (frame) {
> +    nsCOMPtr<nsISupports> container = frame->OwnerDoc()->GetContainer();

Add a check for non-null container here, either with `if (!container)`  or `NS_ENSURE_TRUE`, whichever you prefer (both styles are already used throughout this file)

Comment 15

[Tom S [:evilpie]]

I am currently trying to find out why this patch is causing problems on B2G. Something else we haven't considered yet is, if we should do something like my patch for SetTitle instead of using DOMTitleChanged and the message manager. I guess we still need some way to also set contentTitle on the browser.

Comment 16

[Tom S [:evilpie]]

Attachment: v2.1

I debugged the problem with help from bill. Basically we tried to send the SetStatus before ipc machinery was fully set up.

Comment 17

[Tom S [:evilpie]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/76954219506a
Leave open for a bit so I can take a look at SetTitle.

Comment 18

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/76954219506a

Comment 19

[Tom S [:evilpie]]

I investigated if the SetTitle implementation would make sense, but I does not. I think our implementation based on the messages is good enough (tm). Closing this bug.

Comment 20

[Tom S [:evilpie]]

Added a note that we don't want to support SetTitle
http://hg.mozilla.org/integration/mozilla-inbound/rev/1ff572d74e73

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/1ff572d74e73