Open Bug 1298931 Opened 4 years ago Updated 2 years ago

Pass along full packet in will-navigate and navigate events

Categories

(DevTools :: Framework, defect, P3)

defect

Tracking

(Not tracked)

People

(Reporter: linclark, Unassigned)

Details

Attachments

(1 file)

Currently, the will-navigate and navigate events only pass along specific properties from the tabNavigated packet: https://dxr.mozilla.org/mozilla-central/source/devtools/client/framework/target.js#488

The packet has other properties that are useful for consumers. For example, the new console is switching on packet type to figure out how to access properties of a packet.

The other events emitted in the same function just pass along the packet. The event object created for will-navigate and navigate gets an additional _navPayload property, but it's not clear that this is still being used by anything.

We should try just passing along the packet. If that breaks anything in try, we should pass along the packet with the _navPayload set.
Priority: -- → P3
Comment on attachment 8786066 [details]
Bug 1298931: Pass along full packet in will-navigate and navigate events.

https://reviewboard.mozilla.org/r/75028/#review73176

::: devtools/client/framework/target.js
(Diff revision 3)
> -      return;
> -    }
> -
> -    // emit event if the top frame is navigating
> -    if (progress.isTopLevel) {
> -      // Emit the event if the target is not remoted or store the payload for

Seems like this is OK to remove since it is only emitting if not remoted (which we aren't supporting if I understood the convo we had yesterday correctly).  Going to ask for additional review from jryans just to be sure.
Attachment #8786066 - Flags: review?(bgrinstead) → review+
Comment on attachment 8786066 [details]
Bug 1298931: Pass along full packet in will-navigate and navigate events.

See Comment 4
Attachment #8786066 - Flags: review?(jryans)
Comment on attachment 8786066 [details]
Bug 1298931: Pass along full packet in will-navigate and navigate events.

https://reviewboard.mozilla.org/r/75028/#review73194

Makes sense overall, thanks for removing (this part of) the tire fire!

::: devtools/client/framework/target.js:650
(Diff revision 3)
>   */
>  function TabWebProgressListener(aTarget) {
>    this.target = aTarget;
>  }
>  
>  TabWebProgressListener.prototype = {

`TabWebProgressListener` no longer serves a purpose, so we should remove it entirely.

Looking more generally, the entire `_setupListeners` path should be dead code.  However, the parts outside of `TabWebProgressListener` aren't related to your goal here, so we can leave that for separate work (bug 1072764).
Attachment #8786066 - Flags: review?(jryans) → review+
Comment on attachment 8786066 [details]
Bug 1298931: Pass along full packet in will-navigate and navigate events.

https://reviewboard.mozilla.org/r/75028/#review73224

::: devtools/client/framework/target.js
(Diff revisions 3 - 5)
>    },
>  
>    /**
> -   * Teardown event listeners.
> -   */
> -  _teardownListeners: function () {

We should keep `_teardownListeners` for the 3 tab events at the end (since they are still added by `_setupListeners`.
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.