565388 - Need a way to create invisible, permanent docshells

Status: RESOLVED FIXED

(Core :: Widget, defect)

Description

[:Felipe Gomes (needinfo for replies!)]

Such feature would be useful to many add-ons, and historically the hidden window has been (mis-)used for this purpose. The main point is that sometimes a page would need to be loaded independent of the life-cycle of any particular browser window.
One of the Jetpack APIs would benefit from this, to avoid using the hidden window and doing workarounds on it.


One way to do this would be the ability to create more than 1 hidden window. Do we need a full window to make a docshell work? I don't know exactly what are the options here.  It would also be good to have it implemented in such a way that these docshells will live in the content process for when e10s lands.

Comment 1

[Matthew Tuck [:CodeMachine]]

My extension uses canvas.drawWindow to take a snapshot of an element, would that be a use case of this?

Comment 2

[Myk Melez [:myk] [@mykmelez]]

Matthew: is the element in an existing docshell (web page loaded into a tab, sidebar, chrome browser window, etc.)?  If so, then it wouldn't benefit from the functionality being requested here.

If, however, you need to load the page containing that element in a way that doesn't display the page for the user in order to take that snapshot, then this functionality would be very helpful to you.

Does that make sense?

Comment 3

[Matthew Tuck [:CodeMachine]]

I need to take a snapshot of a translucent panel that's overlaid onto the chrome.  The snapshot should maintain the translucency and not incorporate what's behind.

The reason for this is I have a "flip animation" that emulates something turning around, which necessitates scaling the element in the X-axis ...

In order to do this correctly I currently create a carbon copy of the element in a scratch IFrame that I also overlay into the browser chrome.

The current implementation works as far as it goes, but is really finicky, and I was wondering if this would make it smoother and remove the need to start inserting stuff into the chrome DOM.

Now that I think about it, I also have printing functionality which writes HTML into a scratch IFRAME which might benefit from this too.

Comment 4

[Boris Zbarsky [:bzbarsky]]

So is what's wanted here basically the sort of setup TabChild::Init does?  We could maybe even skip on the widget bits if we don't care about layout inside the nsWebBrowser, though it sounds like at least some of the consumers might care about it?

Comment 5

[Myk Melez [:myk] [@mykmelez]]

bz: I'm unfamiliar with TabChild::Init, and reading through it just now didn't immediately clarify matters, but to address your question, I think that some of the consumers of this API would care about layout.

One hypothetical example is an addon that updates the snapshots of your popular sites on the New Tab page. I could also imagine an addon that uses the Google Maps API (which, the last time I checked, requires you to load a Google Maps page) to generate map images.

I'm sure I used to know of better, and non-hypothetical examples, but somehow I can't think of them right now.  And the very real use case from bug 820953 probably doesn't care about layout.  So if it's easier to get things working without layout first, then I would elect that option.

Also setting needsinfo on Felipe in case he has better examples.

Comment 6

[Boris Zbarsky [:bzbarsky]]

> I'm unfamiliar with TabChild::Init

It's what we use to set up the docshells in the child process in e10s, fwiw.

I'm hoping to get to this bug in the near future, but at this point that might be after the new year.  :(

Comment 7

[Myk Melez [:myk] [@mykmelez]]

Clearing ignored needinfo request.

Comment 8

[Eddy Bruel [:ejpbruel]]

bz pointed out to me that TabChild::Init does almost exactly what we want already. That is, it creates a PuppetWidget, which is a widget that has no associated OS window, and associates that with the doc shell.

The only problem for our use case is that we have no tab child either, and PuppetWidget is currently unable to handle NULL tab childs. I've opened bug 846881 to address this problem.

The second problem is that widgets are currently not scriptable, so there's no way to create a PuppetWidget from within JavaScript. This is easily solved by exposing a function that replicates what TabChild::Init, and returns some usable object (presumably a docshell).

Comment 9

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

Attachment: Pointer to Github pull request: https://github.com/mozilla/addon-sdk/pull/1125

Pointer to Github pull-request

Comment 10

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

Eddy here is simplified test case to reproduce crash:
https://gist.github.com/Gozala/6033211

Comment 11

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

bz wha would be a right way to destroy this windowless docshells ? So that they loaded docs and js within them will be GC-ed ?

Only think I can think of is:

docShell.contentViewer.close();
docShell.contentViewer.destroy();
docShell = null;

But I'm not sure if that's a right way to go about or if some of this is redundant.

Comment 12

[Boris Zbarsky [:bzbarsky]]

I would think that should happen automatically, no?  As in, once you drop the reference to the nsIWebBrowser, it should get GCed.

Comment 13

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

But when does document in it is going to be unloaded ? Or what if something holds reference to an object from the document in it ?

Having a way to destroy it manually would be useful.

Comment 14

[Boris Zbarsky [:bzbarsky]]

> But when does document in it is going to be unloaded ?

When the nsIWebBrowser is GCed.

> Or what if something holds reference to an object from the document in it ?

That would keep the object alive, even if you manually call destroy() on the docshell, obviously.

A bigger concern is what happens if you have a cycle through the objects in the loaded document that keep the nsIWebBrowser alive...

> Having a way to destroy it manually would be useful.

In that case, just destroy() on the nsIWebBrowser should be what you want.

Comment 15

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

Hey Eddy, I just want to point out that we're still using iframe workarounds 
since we're not able to switch to this because it causes crashes as pointed out earlier. Do you by any chance have some update on this ?

Comment 16

[Eddy Bruel [:ejpbruel]]

It's on my to do list for when I get back from the work week. If I have some spare time, I could even look at it during the work week, but no promises there.

Comment 17

[Gabor Krizsanits (INACTIVE)]

(In reply to Irakli Gozilalishvili [:irakli] [:gozala] [@gozala] from comment #10)
> Eddy here is simplified test case to reproduce crash:
> https://gist.github.com/Gozala/6033211

I had some time to look into this and Eddy said he is busy with other bugs right now, so here it is.

scratchpad code:

let Ci = Components.interfaces;
let Cc = Components.classes;
 
let addonPrincipal = Cc["@mozilla.org/systemprincipal;1"].
createInstance(Ci.nsIPrincipal);
let docShell = Cc["@mozilla.org/appshell/appShellService;1"].
getService(Ci.nsIAppShellService).
createWindowlessBrowser().
QueryInterface(Ci.nsIInterfaceRequestor).
getInterface(Ci.nsIDocShell);
 
docShell.createAboutBlankContentViewer(addonPrincipal);
 
let win = docShell.contentViewer.DOMDocument.defaultView;
win.location = "data:application/vnd.mozilla.xul+xml;charset=utf-8,<window/>";

It hits an assertion in nsDocumentOpenInfo::TryContentListener because the content viewer creation fails. Problem is that the channel has null principal and that cannot load xul. Since this all tells me very little, I ask bz and hope he knows what is going on, and what should be fixed...

call stack:

>	xul.dll!MayUseXULXBL(nsIChannel * aChannel=0x1f9e01f0) Line 128	C++
 	xul.dll!nsContentDLF::CreateInstance(const char * aCommand=0x06973718, nsIChannel * aChannel=0x1f9e01f0, nsILoadGroup * aLoadGroup=0x1f1202b8, const char * aContentType=0x1f9e02c0, nsISupports * aContainer=0x19b28c98, nsISupports * aExtraInfo=0x00000000, nsIStreamListener * * aDocListener=0x1f9e05ac, nsIContentViewer * * aDocViewer=0x0034ef3c) Line 236	C++
 	xul.dll!nsDocShell::NewContentViewerObj(const char * aContentType=0x1f9e02c0, nsIRequest * request=0x1f9e01f0, nsILoadGroup * aLoadGroup=0x1f1202b8, nsIStreamListener * * aContentHandler=0x1f9e05ac, nsIContentViewer * * aViewer=0x0034ef3c) Line 8163	C++
 	xul.dll!nsDocShell::CreateContentViewer(const char * aContentType=0x1f9e02c0, nsIRequest * request=0x1f9e01f0, nsIStreamListener * * aContentHandler=0x1f9e05ac) Line 7960	C++
 	xul.dll!nsDSURIContentListener::DoContent(const char * aContentType=0x1f9e02c0, bool aIsContentPreferred=false, nsIRequest * request=0x1f9e01f0, nsIStreamListener * * aContentHandler=0x1f9e05ac, bool * aAbortProcess=0x0034f02a) Line 123	C++
 	xul.dll!nsDocumentOpenInfo::TryContentListener(nsIURIContentListener * aListener=0x1f120438, nsIChannel * aChannel=0x1f9e01f0) Line 681	C++
 	xul.dll!nsDocumentOpenInfo::DispatchContent(nsIRequest * request=0x1f9e01f0, nsISupports * aCtxt=0x00000000) Line 382	C++
 	xul.dll!nsDocumentOpenInfo::OnStartRequest(nsIRequest * request=0x1f9e01f0, nsISupports * aCtxt=0x00000000) Line 258	C++
 	xul.dll!nsBaseChannel::OnStartRequest(nsIRequest * request=0x1f9e8ba8, nsISupports * ctxt=0x00000000) Line 720	C++
 	xul.dll!nsInputStreamPump::OnStateStart() Line 463	C++
 	xul.dll!nsInputStreamPump::OnInputStreamReady(nsIAsyncInputStream * stream=0x1f9e0720) Line 388	C++
 	xul.dll!nsInputStreamReadyEvent::Run() Line 83	C++
 	xul.dll!nsThread::ProcessNextEvent(bool mayWait=false, bool * result=0x0034f366) Line 622	C++
 	xul.dll!NS_ProcessNextEvent(nsIThread * thread=0x00703138, bool mayWait=false) Line 238	C++
 	xul.dll!mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate * aDelegate=0x006fd220) Line 81	C++
 	xul.dll!MessageLoop::RunInternal() Line 221	C++
 	xul.dll!MessageLoop::RunHandler() Line 214	C++
 	xul.dll!MessageLoop::Run() Line 188	C++
 	xul.dll!nsBaseAppShell::Run() Line 163	C++

Comment 18

[Boris Zbarsky [:bzbarsky]]

Well, the channel has null principal because the URI is a data: URI.  And it can't inherit one from the parent window or whatnot because there is no parent to inherit from (and in any case, inheriting the system principal into a non-chrome docshell is disallowed for security reasons).

If you must load XUL from data: URIs in a disconnected window from script, we'll need some new API...  Is the data: URI a hard requirement?

Comment 19

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

(In reply to Boris Zbarsky [:bz] from comment #18)
> Well, the channel has null principal because the URI is a data: URI.  And it
> can't inherit one from the parent window or whatnot because there is no
> parent to inherit from (and in any case, inheriting the system principal
> into a non-chrome docshell is disallowed for security reasons).
>

I see, I assumed that following line would make the trick of providing custom principle:

docShell.createAboutBlankContentViewer(addonPrincipal);


> 
> If you must load XUL from data: URIs in a disconnected window from script,
> we'll need some new API...  Is the data: URI a hard requirement?
>

I don't think data URI is a requirement, it just a simplest option that worked
for us when using using iframes in the hidden window.

What we really need is a way to create a docshell with a custom principal. We also need
to be able to create iframes there with which we'll be able to swap frame loaders, now
that imposes dependency on XUL documents.

Comment 20

[Boris Zbarsky [:bzbarsky]]

> I see, I assumed that following line would make the trick of providing custom principle:

Hmm.  So it should.  Have you stepped through the loadURI call to see why it's not being inherited?

> now that imposes dependency on XUL documents.

Does it?  It imposes a dependency on xul:iframe elements, but you can create those in an HTML document, I suspect.

Comment 21

[Gabor Krizsanits (INACTIVE)]

(In reply to Boris Zbarsky [:bz] from comment #20)
> Hmm.  So it should.  Have you stepped through the loadURI call to see why
> it's not being inherited?

In nsDocShell::GetInheritedPrincipal we have a typeContent as the mItemType that prevents us from inheriting the system principal it seems. And we have an about::blank document to start with if that matters... I guess if we want chrome privileges we should not start with about::blank document, but with something that is already chrome, or am I wrong?

> Does it?  It imposes a dependency on xul:iframe elements, but you can create
> those in an HTML document, I suspect.

I hope we can get rid of that swap frame loader magic from the SDK, but in any case we need here a chrome document for now, xul or HTML that might not be so crucial then.

Comment 22

[Boris Zbarsky [:bzbarsky]]

> we have a typeContent as the mItemType that prevents us from inheriting the system
> principal it seems.

Sure.  But I assumed you were passing a non-system principal to createAboutBlankContentViewer()?  If you're passing a system principal, then yeah, that won't get inherited.

> I guess if we want chrome privileges we should not start with about::blank document, but
> with something that is already chrome

If you want chrome privileges, you shouldn't have a typeContent docshell, imo.  

How about we add an argument to createWindowlessBrowser that indicates whether you want a typeContent or typeChrome docshell?

Comment 23

[Gabor Krizsanits (INACTIVE)]

(In reply to Boris Zbarsky [:bz] from comment #22)
> Sure.  But I assumed you were passing a non-system principal to
> createAboutBlankContentViewer()?  If you're passing a system principal, then
> yeah, that won't get inherited.

Ah yeah, the addonPrincipal var name was a bit misleading in the original example, but it's really just this: let addonPrincipal = Cc["@mozilla.org/systemprincipal;1"].

> How about we add an argument to createWindowlessBrowser that indicates
> whether you want a typeContent or typeChrome docshell?

Sounds good to me. Eddy, do you want to implement this (and if so when?) or shall I take a look at it?

Comment 24

[Gabor Krizsanits (INACTIVE)]

Another exception... this time from ShadowLayerForwarder::BeginTransaction, HasShadowManager() returns null. Looks like it's some kind of paint even that our puppet widget failed to avoid... How can I prevent this?

 	xul.dll!mozilla::layers::ShadowLayerForwarder::BeginTransaction(const nsIntRect & aTargetBounds={...}, mozilla::ScreenRotation aRotation=ROTATION_0, const nsIntRect & aClientBounds={...}, unsigned int aOrientation=0x00000004) Line 202	C++
 	xul.dll!mozilla::layers::ClientLayerManager::BeginTransactionWithTarget(gfxContext * aTarget=0x00000000) Line 138	C++
 	xul.dll!mozilla::layers::ClientLayerManager::BeginTransaction() Line 156	C++
 	xul.dll!PresShell::Paint(nsView * aViewToPaint=0x192b0bf8, const nsRegion & aDirtyRegion={...}, unsigned int aFlags=0x00000001) Line 5603	C++
 	xul.dll!nsViewManager::ProcessPendingUpdatesForView(nsView * aView=0x192b0bf8, bool aFlushDirtyRegion=true) Line 411	C++
 	xul.dll!nsViewManager::ProcessPendingUpdates() Line 1041	C++
 	xul.dll!nsRefreshDriver::Tick(__int64 aNowEpoch=0x0004e6c04a8d1602, mozilla::TimeStamp aNowTime={...}) Line 1206	C++
 	xul.dll!mozilla::RefreshDriverTimer::TickDriver(nsRefreshDriver * driver=0x196d6458, __int64 jsnow=0x0004e6c04a8d1602, mozilla::TimeStamp now={...}) Line 167	C++
 	xul.dll!mozilla::RefreshDriverTimer::Tick() Line 158	C++
 	xul.dll!mozilla::RefreshDriverTimer::TimerTick(nsITimer * aTimer=0x1017e050, void * aClosure=0x1017dfa8) Line 184	C++
 	xul.dll!nsTimerImpl::Fire() Line 546	C++
 	xul.dll!nsTimerEvent::Run() Line 632	C++
 	xul.dll!nsThread::ProcessNextEvent(bool mayWait=false, bool * result=0x002ef24a) Line 622	C++
 	xul.dll!NS_ProcessNextEvent(nsIThread * thread=0x00bed748, bool mayWait=false) Line 238	C++
 	xul.dll!mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate * aDelegate=0x001aa8e0) Line 81	C++

Comment 25

[Boris Zbarsky [:bzbarsky]]

I don't know.  Timothy, do you?

Comment 26

[Timothy Nikkel (:tnikkel)]

PuppetWidgets still need to paint in general. What changes were made to try to make it not paint? Sorry if this has an obvious answer, I only skimmed all of the previous comments on this bug.

Comment 27

[Timothy Nikkel (:tnikkel)]

Or are you asking how to avoid the abort_if_false?

Comment 28

[Boris Zbarsky [:bzbarsky]]

This puppetwidget is just there to make layout kinda happy.  It definitely doesn't need to paint.  But I'm not sure we ever really turned it off.

Comment 29

[Timothy Nikkel (:tnikkel)]

So we want a way to cut off all painting related activity at the source with as little fuss as possible then?

Comment 30

[Boris Zbarsky [:bzbarsky]]

Yep.

Comment 31

[Timothy Nikkel (:tnikkel)]

So perhaps if we can mark the puppet widget somehow (an attribute on some content node maybe? but then how to get that to the widget) the view manager code could check that "don't bother painting" flag on the widget before any kind of painting related activity. Or maybe the view manager could ask the presshell to look at some content node for this check.

Comment 32

[Boris Zbarsky [:bzbarsky]]

This puppet widget is set up directly by C++ code we control, and never needs to paint, so the marking part is easy.  There is no content node involved, though.  But of course we have a docshell we could set flags on or whatnot.

Comment 33

[Timothy Nikkel (:tnikkel)]

Oh then perfect. Once the widget is marked it's probably just a matter of adding a few if's in nsViewManager.cpp.

Comment 34

[Gabor Krizsanits (INACTIVE)]

Attachment: part1: chrome option for the invisible docshell

I guess this is the way to set the docshell in chrome mode... It seems to work... am I close?

Comment 35

[Gabor Krizsanits (INACTIVE)]

Attachment: part2: no paint for invisible docshell

I'm not sure who should I flag for a feedback here... Feel free to pass the ball to Timothy. So I tried flagging the widget, but that would mean I have to flag any subsequent widget related to this docshell probably... so I ended up with this version instead, however, getting to the docshell from nsViewManager is kind of too complicated, and I'm not sure how hot is this code... Is there an easier way? Do I need these null checks or those should be only asserts?

Also I don't know how to write a proper test for this. In the example we do win.location = ... , is there a reliable way to get a notification when the new document is loaded?

Comment 36

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 809912 [details] [diff] [review]
part1: chrome option for the invisible docshell

Looks right, but what's with the added SetInvisible() call?

Also, you could make the argument optional, so the default for no args would be non-chrome.

Comment 37

[Boris Zbarsky [:bzbarsky]]

> Is there a reliable way to get a notification when the new document is loaded?

You can use a web progress listener, right?

Comment 38

[Timothy Nikkel (:tnikkel)]

(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #35)
> So I tried flagging the widget, but that would mean I have
> to flag any subsequent widget related to this docshell probably... so I
> ended up with this version instead, however, getting to the docshell from
> nsViewManager is kind of too complicated, and I'm not sure how hot is this
> code... 

How hard is it to flag the widget from the docshell? I think I would prefer that, unless it's overly complicated.

Comment 39

[Gabor Krizsanits (INACTIVE)]

(In reply to Boris Zbarsky [:bz] from comment #36)
> Comment on attachment 809912 [details] [diff] [review]
> part1: chrome option for the invisible docshell
> 
> Looks right, but what's with the added SetInvisible() call?

whoops... I split the changes into two patches quickly, and that part should belong to part2... I will fix it.

> 
> Also, you could make the argument optional, so the default for no args would
> be non-chrome.

Hmmm... how can I do that in xpidl?

Comment 40

[Boris Zbarsky [:bzbarsky]]

  nsIWebNavigation createWindowlessBrowser([optional] in bool aIsChrome);

Comment 41

[Gabor Krizsanits (INACTIVE)]

(In reply to Timothy Nikkel (:tn) from comment #38)
> (In reply to Gabor Krizsanits [:krizsa :gabor] from comment #35)
> 
> How hard is it to flag the widget from the docshell? I think I would prefer
> that, unless it's overly complicated.

At widget creation time I don't think that can be done. What is the problem with the current one? The best I could come up with is doing it in nsBaseWidget::SetWidgetListener. There I could fetch a reference to the docshell from the widget listener with the same dance like I did it in the current version and set the flag on the widget as well. Would that be any better? Your concern is performance or something else?

Comment 42

[Timothy Nikkel (:tnikkel)]

(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #41)
> (In reply to Timothy Nikkel (:tn) from comment #38)
> > (In reply to Gabor Krizsanits [:krizsa :gabor] from comment #35)
> > 
> > How hard is it to flag the widget from the docshell? I think I would prefer
> > that, unless it's overly complicated.
> 
> At widget creation time I don't think that can be done. What is the problem
> with the current one? The best I could come up with is doing it in
> nsBaseWidget::SetWidgetListener. There I could fetch a reference to the
> docshell from the widget listener with the same dance like I did it in the
> current version and set the flag on the widget as well. Would that be any
> better? Your concern is performance or something else?

The most natural place to store something that says "you don't have to paint this" is in the widget, and the view code is mostly self contained with external references to frames, presshells and widgets, but not to content nodes or docshells. It would be nice to keep view as an intermediary between widgets and layout without talking to other things. I'll think about if there is a better way to do this.

Comment 43

[Gabor Krizsanits (INACTIVE)]

(In reply to Timothy Nikkel (:tn) from comment #42)
> The most natural place to store something that says "you don't have to paint
> this" is in the widget, and the view code is mostly self contained with
> external references to frames, presshells and widgets, but not to content
> nodes or docshells. It would be nice to keep view as an intermediary between
> widgets and layout without talking to other things. I'll think about if
> there is a better way to do this.

Oh, I understand. Well, I would argue that since the information we store is more like: "don't paint any of these / anything at all" maybe we could store it one level higher, on the presshell. It already has IsPaintingSuppressed... Unless you can come up with something better ofc, I just got stuck with the widget approach.

Comment 44

[Timothy Nikkel (:tnikkel)]

(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #43)
> (In reply to Timothy Nikkel (:tn) from comment #42)
> > The most natural place to store something that says "you don't have to paint
> > this" is in the widget, and the view code is mostly self contained with
> > external references to frames, presshells and widgets, but not to content
> > nodes or docshells. It would be nice to keep view as an intermediary between
> > widgets and layout without talking to other things. I'll think about if
> > there is a better way to do this.
> 
> Oh, I understand. Well, I would argue that since the information we store is
> more like: "don't paint any of these / anything at all" maybe we could store
> it one level higher, on the presshell. It already has
> IsPaintingSuppressed... Unless you can come up with something better ofc, I
> just got stuck with the widget approach.

Presshell would be a better place than the docshell, do you have an approach that works okay for that?

Comment 45

[Gabor Krizsanits (INACTIVE)]

Attachment: part2: no paint for invisible docshell. v2

Something like this. I would still keep the invisible flag on the docshell, this makes it easy to set all presshell to non painting mode at creation time, and I find it a usefull info to keep around on it anyway... (invisble docshells are quite special, there is a big chance that sooner or later we need some exception cases for them at some other places too, not just at painting... it's kind of a whack-a-mole game unfortunately)

Comment 46

[Gabor Krizsanits (INACTIVE)]

Eh... forgot to change the iid of nsIPresShell, and move the docshell->SetInvisible(true); line from patch part1 to this patch too (nsAppShellService::CreateWindowlessBrowser). But I guess the concept is clear anyway, so I will fix these and add test too if you find this version good enough to continue...

Comment 47

[Timothy Nikkel (:tnikkel)]

Comment on attachment 811964 [details] [diff] [review]
part2: no paint for invisible docshell. v2

This seems good.

There might be better places (and more) places to put ifs in the view code to avoid even more useless work, but the approach looks good.

Comment 48

[Gabor Krizsanits (INACTIVE)]

Attachment: part1: chrome option for the invisible docshell. v2

Comment 49

[Gabor Krizsanits (INACTIVE)]

Attachment: part2: no paint for invisible docshell. v3

Comment 50

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 814429 [details] [diff] [review]
part1: chrome option for the invisible docshell. v2

r=me

Comment 51

[Timothy Nikkel (:tnikkel)]

Comment on attachment 814431 [details] [diff] [review]
part2: no paint for invisible docshell. v3

This looks good, thank you. Sorry for the delay.

Do you think we should change PresShell::ShouldIgnoreInvalidation to return true if mIsNeverPainting is true instead of making the ShouldIgnoreInvalidation changes in the view manager? Functionally the same, but perhaps a little nicer.

Comment 52

[Timothy Nikkel (:tnikkel)]

Comment on attachment 814431 [details] [diff] [review]
part2: no paint for invisible docshell. v3

>diff --git a/docshell/test/chrome/Makefile.in b/docshell/test/chrome/Makefile.in
>+		test_bug565388.xul \

You've got this random hunk in here though.

Comment 53

[Gabor Krizsanits (INACTIVE)]

Attachment: part3: test. v1

(In reply to Timothy Nikkel (:tn) from comment #51)
> Comment on attachment 814431 [details] [diff] [review]
> Do you think we should change PresShell::ShouldIgnoreInvalidation to return
> true if mIsNeverPainting is true instead of making the
> ShouldIgnoreInvalidation changes in the view manager? Functionally the same,
> but perhaps a little nicer.

Yes, that's a good idea I will do that.

> >diff --git a/docshell/test/chrome/Makefile.in b/docshell/test/chrome/Makefile.in
> >+		test_bug565388.xul \
> 
> You've got this random hunk in here though.

Whoops... that random hunk supposed to be a test that I forgot to add to the patch. And it seems like we have to add tests differently these days... Anyway I just add the test as another patch. I would ask Boris to take a look at the test but since he is on vacation could you review the test for me? It does not do much just follows the steps that caused a crash and use progress listener to see if the navigation has finished successfully.

Comment 54

[Timothy Nikkel (:tnikkel)]

Comment on attachment 816471 [details] [diff] [review]
part3: test. v1

Looks fine to me.

Comment 55

[Gabor Krizsanits (INACTIVE)]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/98fe3c07b042
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/9373baf8aec2
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/9027ca2649fc

Comment 56

[(no longer active)]

Great work!  Can you please post a summary on how to use this API to dev.platform?

Thanks!

Comment 57

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/98fe3c07b042
https://hg.mozilla.org/mozilla-central/rev/9373baf8aec2
https://hg.mozilla.org/mozilla-central/rev/9027ca2649fc

Comment 58

[Gabor Krizsanits (INACTIVE)]

Irakli: When you have time for it to try the new version do let me know if you run into any other issues.

Comment 59

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

I've being discussed issues in the bug 946015, so clearing needinfo here.

Comment 60

[Eddy Bruel [:ejpbruel]]

Comment on attachment 778054 [details]
Pointer to Github pull request: https://github.com/mozilla/addon-sdk/pull/1125

Clearing feedback? for this patch because I don't think it's relevant anymore. Feel free to reset it if I'm wrong.

Comment 61

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

Reopening this issues as issue in the following comment is still not fixed and blocks us from using it
https://bugzilla.mozilla.org/show_bug.cgi?id=946015#c5

Comment 62

[Timothy Nikkel (:tnikkel)]

Probably best to open a new bug for that issue. Re-opening a year+ old bug is going to lead to confusion with target milestone and tracking regressions from any new work done in this same bug.

Comment 63

[Boris Zbarsky [:bzbarsky]]

Indeed.