Closed Bug 617804 Opened 9 years ago Closed 6 years ago

Add platform support for document status

Categories

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

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla26

People

(Reporter: cjones, Assigned: evilpie)

References

(Depends on 1 open bug, Blocks 1 open bug)

Details

(Whiteboard: [e10s])

Attachments

(1 file, 3 obsolete files)

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.
Whiteboard: [e10s]
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.
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?
TabChild::SetTitle is being called and ignored.
This patch is just to unblock bug 615386.
Attachment #496369 - Flags: review?(benjamin)
Duplicate of this bug: 616405
Oops, missed one.  Silencing these bogus notreacheds blocks turning on reftest-ipc.
Blocks: 615386
Attachment #496369 - Flags: review?(benjamin)
Attachment #496369 - Flags: review+
Attachment #496369 - Flags: approval2.0+
Whiteboard: [e10s] → [e10s][approved-patches-landed]
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?
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.
I believe we should use the messagemanager approach and make TabChild::SetTitle a no-op.
Blocks: fxe10s
Attached patch SetStatusWithContext (obsolete) — Splinter Review
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.
Assignee: nobody → evilpies
Status: NEW → ASSIGNED
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
Attached patch v2 (obsolete) — Splinter Review
Thanks, this was a good hint.
Attachment #496369 - Attachment is obsolete: true
Attachment #781087 - Attachment is obsolete: true
Attachment #781160 - Flags: review?(felipc)
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)
Attachment #781160 - Flags: review?(felipc) → review+
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.
Attached patch v2.1Splinter Review
I debugged the problem with help from bill. Basically we tried to send the SetStatus before ipc machinery was fully set up.
Attachment #781160 - Attachment is obsolete: true
Attachment #784583 - Flags: review?(felipc)
Attachment #784583 - Flags: review?(felipc) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/76954219506a
Leave open for a bit so I can take a look at SetTitle.
Whiteboard: [e10s][approved-patches-landed] → [e10s][approved-patches-landed][leave-open]
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.
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Summary: Add platform support (if any) for document status and title → Add platform support for document status
Added a note that we don't want to support SetTitle
http://hg.mozilla.org/integration/mozilla-inbound/rev/1ff572d74e73
Whiteboard: [e10s][approved-patches-landed][leave-open] → [e10s]
Target Milestone: --- → mozilla26
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.