Closed Bug 1159727 Opened 5 years ago Closed 5 years ago

Document TabActor

Categories

(DevTools :: Framework, defect)

defect
Not set

Tracking

(firefox40 fixed)

RESOLVED FIXED
Firefox 40
Tracking Status
firefox40 --- fixed

People

(Reporter: ochameau, Assigned: ochameau)

References

Details

Attachments

(1 file, 1 obsolete file)

I don't think we have any decent document for TabActor,
which is a key to understand many actor related details.
I started writing a document block just on top of it.
Depends on: 1159731
Attached patch patch v1 (obsolete) — Splinter Review
Comment on attachment 8599318 [details] [diff] [review]
patch v1

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

James, Could you confirm me that such doc help discovering TabActor guts?
Attachment #8599318 - Flags: feedback?(jlong)
Comment on attachment 8599318 [details] [diff] [review]
patch v1

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

Wow, this is gold! This helps a *ton*, thanks!
Attachment #8599318 - Flags: feedback?(jlong) → feedback+
Comment on attachment 8599318 [details] [diff] [review]
patch v1

Panos, I think you reviewed most of this (especially during the iframe switching feature landing)?
I tried to gather all my memories about all this, but feel free to contribute ;)
Attachment #8599318 - Flags: review?(past)
Comment on attachment 8599318 [details] [diff] [review]
patch v1

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

Comments, w00t! Although I can't resist being nitpicky about these things, this is really useful!

::: toolkit/devtools/server/actors/webbrowser.js
@@ +608,5 @@
> + * But also track the lifetime of the document being tracked.
> + *
> + * ### Main requests:
> + *
> + * `attach`/`detach` requests allow to starts/stop:

Typo: start/stop

@@ +613,5 @@
> + *  - document watching:
> + *    Starts watching for new documents and emits `tabNavigated` and
> + *    `frameUpdate` over RDP.
> + *  - thread actor:
> + *    Starts instanciating a ThreadActor to allow debugging JS of the targeted

"Instantiates a ThreadActor that can be later attached to in order to debug JS sources in the document."

Also, threadActor doesn't seem to fit very well in a bullet after "allow to start/stop". Maybe something like this?

`attach`/`detach`:
  - start/stop document watching:
    ...
  - retrieve the threadActor:
    Instantiates a ThreadActor...

@@ +619,5 @@
> + * `switchToFrame`:
> + *  Change the targeted document of the whole TabActor, and its child tab actors
> + *  to an iframe or back to its original document.
> + *
> + * Note that most of TabActor properties are meants to be used by the various

"most of the TabActor properties (like ...) are meant to"

@@ +620,5 @@
> + *  Change the targeted document of the whole TabActor, and its child tab actors
> + *  to an iframe or back to its original document.
> + *
> + * Note that most of TabActor properties are meants to be used by the various
> + * child tab actors. (Like `chromeEventHandler` or `docShells`)

Better to put the parenthesis after "properties", because right now it may also refer to "actors".

@@ +626,5 @@
> + * ### RDP events:
> + *
> + *  - `tabNavigated`:
> + *    Sent either when we just start requesting a new URL to be loaded
> + *    in the targeted document, or once this URL is fully loaded.

How about: "Sent when the tab is about to navigate or has just navigated to a different document."

Focuses more on the navigation than the mechanics of URL fetching.

@@ +629,5 @@
> + *    Sent either when we just start requesting a new URL to be loaded
> + *    in the targeted document, or once this URL is fully loaded.
> + *    This event contains the following attributes:
> + *     * url (string) The new URI being loaded.
> + *     * nativeConsoleAPI (boolean) ?

`false` if the console API of the page has been overridden (e.g. by Firebug) or `true` if the Gecko implementation is used.

@@ +631,5 @@
> + *    This event contains the following attributes:
> + *     * url (string) The new URI being loaded.
> + *     * nativeConsoleAPI (boolean) ?
> + *     * state (string) "start" if we just start requesting the URL,
> + *                      "stop" if the URL is done loading.

Nit: "new URL" in both places.

@@ +634,5 @@
> + *     * state (string) "start" if we just start requesting the URL,
> + *                      "stop" if the URL is done loading.
> + *     * isFrameSwitching (boolean) This event is a fake one
> + *                                  dispatched when we switched
> + *                                  the TabActor context to another iframe.

Do we need to mention that this is a fake event here? (I'm not even sure that calling it fake is entirely accurate, it's certainly not emitted by Gecko, but neither are the rest of the tabNavigated events)

Semantics aside, how about: "Indicates the event is dispatched when switching the TabActor context to a different frame"

@@ +639,5 @@
> + *     * title (string) The document title being loaded.
> + *                      (sent only on state=stop)
> + *
> + *  - `frameUpdate`:
> + *    4 different possible shapes for this event related to iframes

It would be useful to add an introductory sentence describing the purpose of the event first. Something like: "Sent when there was a change in the child frames contained in the document or when the tab's context was switched to another frame. This event can have four different forms depending on the type of incident:".

@@ +652,5 @@
> + *      { selected: #id }
> + *
> + * ### Internal, non-rdp events:
> + * Various events are also dispatched on the TabActor itself (that are not
> + * related to RDP, so, not sent to the client). They all relates to the documents

Typo: "They all relate"

Also adding the parentheses is not necessary, the comment can nicely complement the first sentence.

@@ +660,5 @@
> + *    All pending user prompts are dealt with,
> + *    but it is fired before the first request starts.
> + *  - navigate
> + *    This event is fired once the document is loaded,
> + *    after the load event, it's document ready-state is "complete".

"once the document's readyState is..."

@@ +669,5 @@
> + *     * When a page is unfrozen from the bfcache, equivalent of `pageshow`.
> + *       We already received a window-ready for this window,
> + *       but while froze into the bfcache we received a window-destroyed
> + *       and now we are told that the page is live again and should resume
> + *       taking care of it.

This seems clearer to me: "We will have already received a window-ready event for this window when it was created, but we received a window-destroyed event when it was frozen into the bfcache, and now the user navigated back to this page, so it's now live again and we should resume handling it."

@@ +670,5 @@
> + *       We already received a window-ready for this window,
> + *       but while froze into the bfcache we received a window-destroyed
> + *       and now we are told that the page is live again and should resume
> + *       taking care of it.
> + *     * For each existing documents, when `attach` request is called.

"For each existing doument, when an `attach` request is received."

@@ +672,5 @@
> + *       and now we are told that the page is live again and should resume
> + *       taking care of it.
> + *     * For each existing documents, when `attach` request is called.
> + *       It is obviously late in process and may require reload
> + *       if you have to hook before the page script are loaded.

Not entirely sure of what you are trying to highlight here (break-on-load-event uses window-ready), but perhaps this covers it: "At this point scripts in the page will be already loaded."

@@ +676,5 @@
> + *       if you have to hook before the page script are loaded.
> + *  - window-destroyed
> + *    This event is fired in two cases:
> + *     * When the window object is destroyed, i.e. when the related document
> + *       is garbage collected. When the tab is closed or the iframe is removed

When... When... sounds like a bulleted list. I'd change the second sentence to start with "This can happen when the tab is closed..."

@@ +677,5 @@
> + *  - window-destroyed
> + *    This event is fired in two cases:
> + *     * When the window object is destroyed, i.e. when the related document
> + *       is garbage collected. When the tab is closed or the iframe is removed
> + *       from the DOM. It is equivalent of `inner-window-destroyed` event.

"equivalent to the `inner-window-destroyed` event"

@@ +679,5 @@
> + *     * When the window object is destroyed, i.e. when the related document
> + *       is garbage collected. When the tab is closed or the iframe is removed
> + *       from the DOM. It is equivalent of `inner-window-destroyed` event.
> + *     * When the page goes into the bfcache and gets frozen.
> + *       Equivalent of `pagehide`.

"The equivalent of"

@@ +680,5 @@
> + *       is garbage collected. When the tab is closed or the iframe is removed
> + *       from the DOM. It is equivalent of `inner-window-destroyed` event.
> + *     * When the page goes into the bfcache and gets frozen.
> + *       Equivalent of `pagehide`.
> + * Note that *all* these events are also dispatched in the following order

"also" is superfluous in this case.

@@ +685,5 @@
> + * when we switch the context of the TabActor to a given iframe:
> + *   will-navigate, window-destroyed, window-ready, navigate
> + * An event is also dispatched when doing this context switch,
> + * between window-destroyed and window-ready events:
> + *  - changed-toplevel-document

Why put this event outside of the above list? I would put it at the end (after window-destroyed) and also include it in the parting comment about the order of events on frame switch. In that case I would incorporate the preceding comment ("it is dispatched between window-destroyed and window-ready") into the event comment below.
Attachment #8599318 - Flags: review?(past) → review+
(In reply to Panos Astithas [:past] from comment #5)
> Comment on attachment 8599318 [details] [diff] [review]
> patch v1
> 
> Review of attachment 8599318 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Comments, w00t! Although I can't resist being nitpicky about these things,
> this is really useful!

Thanks, feedback is more than welcome!

> @@ +626,5 @@
> > + * ### RDP events:
> > + *
> > + *  - `tabNavigated`:
> > + *    Sent either when we just start requesting a new URL to be loaded
> > + *    in the targeted document, or once this URL is fully loaded.
> 
> How about: "Sent when the tab is about to navigate or has just navigated to
> a different document."
> 
> Focuses more on the navigation than the mechanics of URL fetching.

It is quite important to understand what is the very precise state of the targeted document,
but this is well described in `state` description, so I switched to your suggestion.

> @@ +634,5 @@
> > + *     * state (string) "start" if we just start requesting the URL,
> > + *                      "stop" if the URL is done loading.
> > + *     * isFrameSwitching (boolean) This event is a fake one
> > + *                                  dispatched when we switched
> > + *                                  the TabActor context to another iframe.
> 
> Do we need to mention that this is a fake event here? (I'm not even sure
> that calling it fake is entirely accurate, it's certainly not emitted by
> Gecko, but neither are the rest of the tabNavigated events)
> 
> Semantics aside, how about: "Indicates the event is dispatched when
> switching the TabActor context to a different frame"

Same thing, it is important to acknowlege that there is no navigation/document load here!
So I amended your sentence with:
 When we switch to an iframe, there is no document load.
 The targeted document is most likely going to be already done loading.
 
> @@ +669,5 @@
> > + *     * When a page is unfrozen from the bfcache, equivalent of `pageshow`.
> > + *       We already received a window-ready for this window,
> > + *       but while froze into the bfcache we received a window-destroyed
> > + *       and now we are told that the page is live again and should resume
> > + *       taking care of it.
> 
> This seems clearer to me: "We will have already received a window-ready
> event for this window when it was created, but we received a
> window-destroyed event when it was frozen into the bfcache, and now the user
> navigated back to this page, so it's now live again and we should resume
> handling it."

Yep, much clearer!!

> @@ +672,5 @@
> > + *       and now we are told that the page is live again and should resume
> > + *       taking care of it.
> > + *     * For each existing documents, when `attach` request is called.
> > + *       It is obviously late in process and may require reload
> > + *       if you have to hook before the page script are loaded.
> 
> Not entirely sure of what you are trying to highlight here
> (break-on-load-event uses window-ready), but perhaps this covers it: "At
> this point scripts in the page will be already loaded."

I tried to emphasise that the document is most likely already fully loaded...
but it may be clearer with your shorter sentence ;)

> @@ +685,5 @@
> > + * when we switch the context of the TabActor to a given iframe:
> > + *   will-navigate, window-destroyed, window-ready, navigate
> > + * An event is also dispatched when doing this context switch,
> > + * between window-destroyed and window-ready events:
> > + *  - changed-toplevel-document
> 
> Why put this event outside of the above list? I would put it at the end
> (after window-destroyed) and also include it in the parting comment about
> the order of events on frame switch. In that case I would incorporate the
> preceding comment ("it is dispatched between window-destroyed and
> window-ready") into the event comment below.

Much better. I had a hard time finding a right way to explain this event vs the others.
Attached patch patch v2Splinter Review
Attachment #8599318 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/87cfa9c94081
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 40
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.