Closed Bug 978058 Opened 6 years ago Closed 6 years ago

Make the Developer HUD support activity windows with transitions

Categories

(Firefox OS Graveyard :: Gaia::System, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
1.4 S3 (14mar)

People

(Reporter: gerard-majax, Assigned: janx)

Details

Attachments

(2 files, 3 obsolete files)

We can make developper HUD disapper as we want.

STR:
 0. Make sure developper HUD is enabled (at least errors, warnings, reflows and jank).
 1. Install "Fil@Tours" from the Marketplace
 2. Start it
 3. Tap "Search", then in the new window, "Select from map"
 4. It opens the 'select-stop' activity, with a transition.

Expected:
 Developper HUD is still there

Actual:
 Developper HUD is lost, and no way to get it back


Launching the same activity without transition, from the main screen by tapping on the "Stops map" or "Where am I" buttons is okay.
Nice catch! Activity windows with transitions are not supported by the Developer HUD yet.

The problem comes from the fact that with transitions, activity windows create a new appwindow iframe with the same `mozapp` attribute as the parent app. This breaks the Developer HUD, because it assumes a specific mozapp attribute corresponds to only one iframe.

Activity windows without transitions seem to reuse the same iframe, so that case is not an issue.
Assignee: nobody → janx
Status: NEW → ASSIGNED
Summary: Let's make Developper HUD disappear → Make the Developer HUD support activity windows with transitions
Is it expected that we loose the HUD even after leaving the activity?
Flags: needinfo?(janx)
Short answer: Yes.

Long answer: Yeeeeeeeees. The Developer HUD currently assumes that `mozapp` attributes are unique, and listens to the creation/destruction of iframes:
- When the main app iframe is created with a `mozapp`, the Developer HUD starts tracking the app.
- When the activity iframe is created with the same `mozapp`, the HUD ignores it, but is still tracking the main app behind the activity.
- When the activity is closed, the HUD stops tracking the app completely, because an iframe with that `mozapp` was destroyed.
Flags: needinfo?(janx)
Attached file gaia pull request
Attachment #8383868 - Flags: review?(21)
Attachment #8383864 - Flags: review?(21)
Comment on attachment 8383864 [details] [diff] [review]
Make Firefox OS' devtools track frames instead of app manifests. r=vingtetun

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

Closed but I would like to see a modified sendEvent before r+ing.

::: b2g/chrome/content/devtools.js
@@ +90,5 @@
>    uninit: function dwp_uninit() {
>      if (!this._client)
>        return;
>  
> +    for (let frame of this._stats.keys()) {

_stats ?

@@ +116,5 @@
>      // FIXME(Bug 962577) Factor getAppActor out of webappsActor.
>      this._client.request({
>        to: this._webappsActor,
>        type: 'getAppActor',
>        manifestURL: manifestURL

Maybe you don't need the intermediate variable and can just use frame.appManifestURL here ?

@@ +217,5 @@
>  
> +    let event = getContentWindow().document.createEvent('CustomEvent');
> +    let payload = Cu.cloneInto(data, this.frame);
> +    event.initCustomEvent('developer-hud-update', true, true, payload);
> +    this.frame.dispatchEvent(event);

Maybe you can just modify the sendEvent method so it check if |content| has a document property, or try to use ownerDocument otherwise ? Or even just |content| if this is a document.

Then you can also rename |content| -> |target|
Comment on attachment 8383868 [details] [review]
gaia pull request

r+ with a small question.
Attachment #8383868 - Flags: review?(21) → review+
Thanks for the review!

(In reply to Vivien Nicolas (:vingtetun) (:21) from comment #6)
> Maybe you can just modify the sendEvent method so it check if |content| has
> a document property, or try to use ownerDocument otherwise ? Or even just
> |content| if this is a document.
> 
> Then you can also rename |content| -> |target|

What about:

> sendEvent: function shell_sendEvent(target, type, details) {
>   let doc = target.document || target.ownerDocument || target;
>   [...]
>   target.dispatchEvent(event);
> },

?
Flags: needinfo?(21)
Better snippet:

> sendEvent: function shell_sendEvent(target, type, details) {
>   let doc = target.document || target.ownerDocument || target;
>   let event = doc.createEvent('CustomEvent');
>   [...]
>   target.dispatchEvent(event);
> },

I'm using any document I can find on `target` to create the event.

Also, using this modified `sendEvent` still requires that I use `Cu.cloneInto(details, frame)` in devtools.js.
(In reply to Jan Keromnes [:janx] from comment #8)
> What about:
> 
> > sendEvent: function shell_sendEvent(target, type, details) {
> >   let doc = target.document || target.ownerDocument || target;
> >   [...]
> >   target.dispatchEvent(event);
> > },
> 
> ?

Sounds OK to me.
Flags: needinfo?(21)
Comment on attachment 8384645 [details] [diff] [review]
Make Firefox OS' devtools track frames instead of app manifests. r=vingtetun

Addressed nits, listened to 'remote-browser-shown' instead of 'remote-browser-pending' to avoid race conditions in future changes, and refactored `send{,Custom}Event` methods a bit.
Attachment #8384645 - Flags: review?(21)
Comment on attachment 8384645 [details] [diff] [review]
Make Firefox OS' devtools track frames instead of app manifests. r=vingtetun

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

Sounds good now.

::: b2g/chrome/content/shell.js
@@ +545,5 @@
>      }
>    },
>  
> +  // Send an event to a specified target, which can be a window, iframe or
> +  // document.

which can be any arbitrary element.
Attachment #8384645 - Flags: review?(21) → review+
Comment on attachment 8384747 [details] [diff] [review]
Make Firefox OS' devtools track frames instead of app manifests. r=vingtetun

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

21's r+
Attachment #8384747 - Flags: review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/93443ede8152
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → 1.4 S3 (14mar)
You need to log in before you can comment on or make changes to this bug.