(no longer active) Assignee
	Description • 9 years ago

Fennec's reload button seems to be operating in "Shift-Reload" mode <http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/browser.js#1718>.  The service worker spec mandates the implementation to bypass the service worker, therefore we get into the situation where in air plane mode, I can load "Trained to Thrill" fine, but when I press reload, I get a Server Not Found error because we bypass the service worker.

The same problem also exists in the "Request deskop mode" feature: <http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/browser.js#3608>

	(no longer active) Assignee
	Comment 1 • 9 years ago

This blocks v1 if we're going to ship on Fennec too.

	Josh Matthews [:jdm]
	Comment 3 • 9 years ago

See also bug 1202052.

	(no longer active) Assignee
	Comment 4 • 9 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f9ead63dc34d

	(no longer active) Assignee
	Comment 5 • 9 years ago

Looks like bug 1202052 has more discussion on this...

	Mark Finkle (:mfinkle) (use needinfo?)
	Comment 6 • 9 years ago

Comment on attachment 8663360 [details] [diff] [review]
Stop bypassing the cache (and service workers) when reloading content in Fennec

Dropping r?until we figure out the UX in bug 1202052

	:Margaret Leibovic
	Comment 7 • 9 years ago

I'm un-duping, because I think we should just land this patch on its own, and figure out how to give users back a "shift reload" functionality over in bug 1202052.

	:Margaret Leibovic
	Comment 8 • 9 years ago

Comment on attachment 8663360 [details] [diff] [review]
Stop bypassing the cache (and service workers) when reloading content in Fennec

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

I think it would be fine to land this and let it bake on Nightly for a bit. We could also make a post to mobile-firefox-dev to ask Nightly testers to keep an eye out for issues with reload functionality.

	(no longer active) Assignee
	Comment 9 • 9 years ago

Thanks, Margaret!  I think this unblocks service workers.  :-)

	Pulsebot
	Comment 10 • 9 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/802679c08c17

	Carsten Book [:Tomcat]
	Comment 11 • 9 years ago

https://hg.mozilla.org/mozilla-central/rev/802679c08c17

	Pulsebot
	Comment 12 • 9 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/e15e24b44323

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 13 • 9 years ago

https://hg.mozilla.org/mozilla-central/rev/e15e24b44323

	(no longer active) Assignee
	Comment 14 • 9 years ago

Comment on attachment 8663360 [details] [diff] [review]
Stop bypassing the cache (and service workers) when reloading content in Fennec

Approval Request Comment
[Feature/regressing bug #]: This is needed if we want to ship Service Workers on Fennec 43.
[User impact if declined]: Comment 0.
[Describe test coverage new/current, TreeHerder]: Tested locally.
[Risks and why]: Some people were worried in bug 1202052 that this may break people's workflows in ways that we don't expect.  AFAIK nobody has filed a bug about that on Nightly yet, but bug 1202052 landing on top of it on Nightly may make the issue less severe on 44.  The issue with this patch is that it impacts non-serviceworker related things, so the risk on the uplift needs to be evaluated accordingly.
[String/UUID change made/needed]: None.

	Brendan Dahl [:bdahl]
	Comment 15 • 9 years ago

This is still broken.

STR:
1) Open https://people.mozilla.org/~bdahl/sandbox/service_worker_cache/go.html
2) Click Register
3) Use the browser reload button

Results:
ServiceWorker controller: null

Expected:
ServiceWorker controller: [object ServiceWorker]

If you use the reload button on the page itself it will work correctly.

I haven't verified yet, but as I mentioned in the other bug the flags should be LOAD_FLAGS_NONE

	Brendan Dahl [:bdahl]
	Comment 16 • 9 years ago

Tried it locally, with LOAD_FLAGS_NONE it behaves as expected.

	Ben Kelly [:bkelly, not reviewing]
	Comment 17 • 9 years ago

Brendan, I just tried this in 44.0a1 (2015-09-30) and I can't reproduce the failure you describe in comment 15.  This was with e10s.

What version and platform did you test on?

	Ben Kelly [:bkelly, not reviewing]
	Comment 18 • 9 years ago

Oh, this is only fennec.  Sorry for my confusion.  I was testing nightly.  Moving this to block the android ship bug.

	(no longer active) Assignee
	Comment 19 • 9 years ago

Comment on attachment 8663360 [details] [diff] [review]
Stop bypassing the cache (and service workers) when reloading content in Fennec

Ah you're right Brendan... I'm not sure why I previously thought that this patch fixes anything.  Let me submit a new one.

	Sebastian Kaspari (:sebastian; :pocmo)
	Comment 21 • 9 years ago

Comment on attachment 8668720 [details] [diff] [review]
Use LOAD_FLAGS_NONE unless shift-reload has been requested

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

::: mobile/android/chrome/content/browser.js
@@ -1727,5 @@
>            browser.gotoIndex(index);
>            break;
>  
>        case "Session:Reload": {
> -        let flags = Ci.nsIWebNavigation.LOAD_FLAGS_BYPASS_PROXY;

Shouldn't we move this ...

@@ -1734,5 @@
>          if (aData) {
>            let data = JSON.parse(aData);
>  
>            if (data.bypassCache) {
>              flags |= Ci.nsIWebNavigation.LOAD_FLAGS_BYPASS_CACHE;

... to here?

	Liz Henry (:lizzard Please n-i to RyanVM, jcristau, or pascal)
	Comment 22 • 9 years ago

Sounds like bug 1202052 is waiting on this to uplift to aurora. If that's the case, then, when this has a patch ready for uplift, please nominate it and let me know. Thanks!

	(no longer active) Assignee
	Comment 23 • 9 years ago

(In reply to Sebastian Kaspari (:sebastian) from comment #21)
> ::: mobile/android/chrome/content/browser.js
> @@ -1727,5 @@
> >            browser.gotoIndex(index);
> >            break;
> >  
> >        case "Session:Reload": {
> > -        let flags = Ci.nsIWebNavigation.LOAD_FLAGS_BYPASS_PROXY;
> 
> Shouldn't we move this ...
> 
> @@ -1734,5 @@
> >          if (aData) {
> >            let data = JSON.parse(aData);
> >  
> >            if (data.bypassCache) {
> >              flags |= Ci.nsIWebNavigation.LOAD_FLAGS_BYPASS_CACHE;
> 
> ... to here?

Sure.

	(no longer active) Assignee
	Comment 25 • 9 years ago

Comment on attachment 8671049 [details] [diff] [review]
Use LOAD_FLAGS_NONE unless shift-reload has been requested

Approval Request Comment
See comment 14, please.

	Pulsebot
	Comment 26 • 9 years ago

https://hg.mozilla.org/mozilla-central/rev/8c3b3ff67448

	Carsten Book [:Tomcat]
	Comment 27 • 9 years ago

https://hg.mozilla.org/mozilla-central/rev/8c3b3ff67448

	Liz Henry (:lizzard Please n-i to RyanVM, jcristau, or pascal)
	Comment 28 • 9 years ago

Ioana, can your team verify the fix here and in bug 1202052 on Nightly?  

Then we can uplift the patches from both bugs at some point next week.

	Sebastian Kaspari (:sebastian; :pocmo)
	Comment 29 • 9 years ago

(In reply to Ehsan Akhgari (don't ask for review please) from comment #25)
> Comment on attachment 8671049 [details] [diff] [review]
> Use LOAD_FLAGS_NONE unless shift-reload has been requested
> 
> Approval Request Comment
> See comment 14, please.

I'm not sure if we can uplift this? The patch is based on changes done in the previous (pushed but not uplifted) patch and some changes I did in bug 1202052 (also not uplifted yet). Maybe we should create a combined patch of the previous patch here, the new patch here and the changes in bug 1202052?

	(no longer active) Assignee
	Comment 30 • 9 years ago

(In reply to Sebastian Kaspari (:sebastian) from comment #29)
> (In reply to Ehsan Akhgari (don't ask for review please) from comment #25)
> > Comment on attachment 8671049 [details] [diff] [review]
> > Use LOAD_FLAGS_NONE unless shift-reload has been requested
> > 
> > Approval Request Comment
> > See comment 14, please.
> 
> I'm not sure if we can uplift this? The patch is based on changes done in
> the previous (pushed but not uplifted) patch and some changes I did in bug
> 1202052 (also not uplifted yet). Maybe we should create a combined patch of
> the previous patch here, the new patch here and the changes in bug 1202052?

Bug 1202052 is waiting for approval.  Once it gets it, we can uplift this on top of it, no?

(If you meant the patches not applying cleanly etc, I can land it on Aurora manually.)

	Sebastian Kaspari (:sebastian; :pocmo)
	Comment 31 • 9 years ago

(In reply to Ehsan Akhgari (don't ask for review please) from comment #30)
> Bug 1202052 is waiting for approval.  Once it gets it, we can uplift this on
> top of it, no?
> 
> (If you meant the patches not applying cleanly etc, I can land it on Aurora
> manually.)

Okay! I was just concerned about us having landed multiple patches here and one in bug 1202052, but only 2 uplift approval requests. :)

	Liz Henry (:lizzard Please n-i to RyanVM, jcristau, or pascal)
	Comment 32 • 9 years ago

Comment on attachment 8671049 [details] [diff] [review]
Use LOAD_FLAGS_NONE unless shift-reload has been requested

OK to uplift along with its companion patch in bug 1202052.  
This blocks service workers for 43.

	Carsten Book [:Tomcat]
	Comment 33 • 9 years ago

Hi ehsan, this cause problems when uplifting to aurora:

merging mobile/android/chrome/content/browser.js
warning: conflicts during merge.
merging mobile/android/chrome/content/browser.js incomplete! (edit conflicts, then use 'hg resolve --mark')

could you take a look, thanks!

	(no longer active) Assignee
	Comment 34 • 9 years ago

https://hg.mozilla.org/releases/mozilla-aurora/rev/01e92f8049cc