1024105 - 'content-page-shown' observer notification doesn't pass 'persisted' data param

Status: NEW

(Firefox :: General, defect)

Description

[Tomislav Jovanovic :zombie]

for bug 1023326, Irakli wants to use observers instead of events, but 'content-page-shown' notification doesn't carry the `persisted` param.

i tracked down where the notification is sent, without the `aPersisted` param:
http://dxr.mozilla.org/mozilla-central/source/content/base/src/nsDocument.cpp#8801

Irakli, can you confirm this is a thing we should do?

Comment 1

[Tomislav Jovanovic :zombie]

Gabor, are you able to please take this bug? (i'm not really a c++ hacker) 

or maybe you can find someone on the platform team? (i know the product/component is wrong)

Comment 2

[Tomislav Jovanovic :zombie]

Irakli thinks a separate notification like `content-page-restored` might be better than showing a bool flag through a string-only data param..

also, i just saw Gabor is on PTO for a month..  :(

Comment 3

[Tomislav Jovanovic :zombie]

(In reply to Tomislav Jovanovic [:zombie] from comment #2)
> also, i just saw Gabor is on PTO for a month..  :(

that's not right.. i'm happy for Gabor! ;)

Comment 4

[Gabor Krizsanits (INACTIVE)]

The best I could do here is to carry over that boolean flag as a string (like: "true"/"false"). I'm not sure we are trying to do the right thing here. This bug does not explain why we need this flag to be carried over, and I don't get the P1 on the bug either.

Comment 5

[Tomislav Jovanovic :zombie]

> The best I could do here is to carry over that boolean flag as a string (like: "true"/"false"). 

that was my initial plan. Irakli, can you post why you don't think that's the best approach?


> I'm not sure we are trying to do the right thing here. 

why is that? can you please elaborate?


> This bug does not explain why we need this flag to be carried over,
> and I don't get the P1 on the bug either.

sorry, that's my fault. in bug 1023326, we are switching to using frame scripts to get tab content events (ready/load/pageshow), because our current code doesn't work with e10s.  Irakli would prefer to use observer notifications instead of events. the problem is, our current API for pageshow also exposes if the page being shown is coming from BFCache (aPersisted) or not [1].

the P1 is just because it's e10s-related, though i don't think this is urgent as i can work around it using events for now.

[1] https://developer.mozilla.org/en-US/Add-ons/SDK/High-Level_APIs/tabs#pageshow

Comment 6

[Gabor Krizsanits (INACTIVE)]

(In reply to Tomislav Jovanovic [:zombie] from comment #5)
> > The best I could do here is to carry over that boolean flag as a string (like: "true"/"false"). 

Actually a stringified object might be better... like "{ persisted: true }"
 
> > I'm not sure we are trying to do the right thing here. 
> 
> why is that? can you please elaborate?

Because observer notifications do not really support carrying over properties like events it seems. Only as byte streams, which seems like a hacky last resort. What's wrong with events other than Irakli prefers notifications? Is that a general preference or are there real issues with events?

> 
> the P1 is just because it's e10s-related, though i don't think this is
> urgent as i can work around it using events for now.

If it can be easily worked around then it does not sound like P1 to me. But I'm not involved in the process of setting priorities so my word does not count much.

Comment 7

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

The reasons I recommended to use of observer notifications over event handlers were:

1. Setting an event listener on docshell.chromeEventHandler is not fully equivalent to use of observer notifications, since it won't include events from nested docshells when there is a chrome document somewhere in the middle of the chain.
2. Observer service let's us use a weak listeners which makes it easier to leverage GC rather than us trying to keep track of lifetimes of two different components in order to resolve cycles.
3. Observer service notifications are dispatched prior to the event dispatching and there for are observers are called prior to any event listener regardless of the order listeners were registered in.

I believe I mentioned few more reasons but I can not recall them right now, overall our past experience with doshell.chromeEventHandler has being error prone and problematic due to error to see one or more behavioral detail list above, while use of observer notifications proved to be a lot more reliable since there are no behavioral specifics as ones mentioned above.

All that being said I do agree with Gabor that this does not need to be a P1 since we do have a workaround we can use until this is fixed.

Comment 8

[Erik Vold [:erikvold] (please needinfo? me)]

This is a platform bug.