Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1931583004:
Prevent Blimp from reloading default URL on resume from minimized state (Closed)
|
Created:
4 years, 7 months ago by shaktisahu Modified:
4 years, 7 months ago CC:
chromium-reviews, anandc+watch-blimp_chromium.org, maniscalco+watch-blimp_chromium.org, sriramsr+watch-blimp_chromium.org, nyquist+watch-blimp_chromium.org, marcinjb+watch-blimp_chromium.org, jessicag+watch-blimp_chromium.org, kmarshall+watch-blimp_chromium.org, dtrainor+watch-blimp_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionPrevent Blimp from reloading default URL on resume from minimized state
Currently, when Blimp loads the default URL when it is launched from
background. This patch attempts to prevent this.
BUG=606415
Committed: https://crrev.com/ad74de2b929f777cf734c78a132d0caeea315973
Cr-Commit-Position: refs/heads/master@{#391626}
Patch Set 1 #
Total comments: 2
Patch Set 2 : Addressed comments #
Total comments: 2
Patch Set 3 : Fixed a condition #
Total comments: 4
Patch Set 4 : Addressed nits #Messages
Total messages: 26 (10 generated)
shaktisahu@chromium.org changed reviewers: + khushalsagar@chromium.org, nyquist@chromium.org
https://codereview.chromium.org/1931583004/diff/1/blimp/client/app/android/ja... File blimp/client/app/android/java/src/org/chromium/blimp/BlimpRendererActivity.java (right): https://codereview.chromium.org/1931583004/diff/1/blimp/client/app/android/ja... blimp/client/app/android/java/src/org/chromium/blimp/BlimpRendererActivity.java:168: if (getUrlFromIntent(intent) != null) { This is making the code really hard to follow. Let's pass the intent directly to handleUrl, both in the startup complete case and new intent case, you can rename it handleUrlFromIntent. Add a bool to keep track of when we load the first Url and only load the default url if the intent did not have a url and this is the first time we got a call to handle the url request. You can explain this in the comments in the code and that will make it easy to understand the flow.
https://codereview.chromium.org/1931583004/diff/1/blimp/client/app/android/ja... File blimp/client/app/android/java/src/org/chromium/blimp/BlimpRendererActivity.java (right): https://codereview.chromium.org/1931583004/diff/1/blimp/client/app/android/ja... blimp/client/app/android/java/src/org/chromium/blimp/BlimpRendererActivity.java:168: if (getUrlFromIntent(intent) != null) { On 2016/04/29 04:37:30, Khushal wrote: > This is making the code really hard to follow. Let's pass the intent directly to > handleUrl, both in the startup complete case and new intent case, you can rename > it handleUrlFromIntent. > > Add a bool to keep track of when we load the first Url and only load the default > url if the intent did not have a url and this is the first time we got a call to > handle the url request. You can explain this in the comments in the code and > that will make it easy to understand the flow. Done.
https://codereview.chromium.org/1931583004/diff/20001/blimp/client/app/androi... File blimp/client/app/android/java/src/org/chromium/blimp/BlimpRendererActivity.java (right): https://codereview.chromium.org/1931583004/diff/20001/blimp/client/app/androi... blimp/client/app/android/java/src/org/chromium/blimp/BlimpRendererActivity.java:205: mLoadUrlForFirstTime = false; Doesn't this need to set to false even in the case below as well? The following case is possible: 1) We start the application with an intent where the url was specified, so we loaded the first page. 2) We receive another intent with no url, since the mLoadUrlForFirstTime flag has not been set to false we will proceed to load the default url. The mLoadUrlForFirstTime flag is basically tracking the first time we enter this method after application startup. You can set it to false before the if block.
https://codereview.chromium.org/1931583004/diff/20001/blimp/client/app/androi... File blimp/client/app/android/java/src/org/chromium/blimp/BlimpRendererActivity.java (right): https://codereview.chromium.org/1931583004/diff/20001/blimp/client/app/androi... blimp/client/app/android/java/src/org/chromium/blimp/BlimpRendererActivity.java:205: mLoadUrlForFirstTime = false; On 2016/05/03 19:57:59, Khushal wrote: > Doesn't this need to set to false even in the case below as well? > The following case is possible: > > 1) We start the application with an intent where the url was specified, so we > loaded the first page. > 2) We receive another intent with no url, since the mLoadUrlForFirstTime flag > has not been set to false we will proceed to load the default url. > > The mLoadUrlForFirstTime flag is basically tracking the first time we enter this > method after application startup. You can set it to false before the if block. Oh.. I missed that
lgtm. Thanks! https://codereview.chromium.org/1931583004/diff/40001/blimp/client/app/androi... File blimp/client/app/android/java/src/org/chromium/blimp/BlimpRendererActivity.java (right): https://codereview.chromium.org/1931583004/diff/40001/blimp/client/app/androi... blimp/client/app/android/java/src/org/chromium/blimp/BlimpRendererActivity.java:200: if (!mLoadUrlForFirstTime && url == null) return; nit: Could you use mFirstUrlLoadDone, initialize it with false instead and set it to true below? Easier to read. :)
https://codereview.chromium.org/1931583004/diff/40001/blimp/client/app/androi... File blimp/client/app/android/java/src/org/chromium/blimp/BlimpRendererActivity.java (right): https://codereview.chromium.org/1931583004/diff/40001/blimp/client/app/androi... blimp/client/app/android/java/src/org/chromium/blimp/BlimpRendererActivity.java:200: if (!mLoadUrlForFirstTime && url == null) return; On 2016/05/03 22:44:22, Khushal wrote: > nit: Could you use mFirstUrlLoadDone, initialize it with false instead and set > it to true below? Easier to read. :) Then you can use if(mFirstUrlLoadDone...) instead of if(!mLoadUrlForFirstTime...).
lgtm
https://codereview.chromium.org/1931583004/diff/40001/blimp/client/app/androi... File blimp/client/app/android/java/src/org/chromium/blimp/BlimpRendererActivity.java (right): https://codereview.chromium.org/1931583004/diff/40001/blimp/client/app/androi... blimp/client/app/android/java/src/org/chromium/blimp/BlimpRendererActivity.java:200: if (!mLoadUrlForFirstTime && url == null) return; On 2016/05/03 22:44:22, Khushal wrote: > nit: Could you use mFirstUrlLoadDone, initialize it with false instead and set > it to true below? Easier to read. :) Done. https://codereview.chromium.org/1931583004/diff/40001/blimp/client/app/androi... blimp/client/app/android/java/src/org/chromium/blimp/BlimpRendererActivity.java:200: if (!mLoadUrlForFirstTime && url == null) return; On 2016/05/03 22:44:22, Khushal wrote: > nit: Could you use mFirstUrlLoadDone, initialize it with false instead and set > it to true below? Easier to read. :) Done.
The CQ bit was checked by shaktisahu@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from khushalsagar@chromium.org Link to the patchset: https://codereview.chromium.org/1931583004/#ps60001 (title: "Addressed nits")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1931583004/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1931583004/60001
The CQ bit was unchecked by commit-bot@chromium.org
Failed to apply the patch.
The CQ bit was unchecked by commit-bot@chromium.org
The CQ bit was checked by shaktisahu@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1931583004/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1931583004/60001
The CQ bit was unchecked by commit-bot@chromium.org
Failed to apply the patch.
The CQ bit was unchecked by commit-bot@chromium.org
The CQ bit was checked by shaktisahu@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1931583004/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1931583004/60001
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Prevent Blimp from reloading default URL on resume from minimized state Currently, when Blimp loads the default URL when it is launched from background. This patch attempts to prevent this. BUG=606415 ========== to ========== Prevent Blimp from reloading default URL on resume from minimized state Currently, when Blimp loads the default URL when it is launched from background. This patch attempts to prevent this. BUG=606415 Committed: https://crrev.com/ad74de2b929f777cf734c78a132d0caeea315973 Cr-Commit-Position: refs/heads/master@{#391626} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/ad74de2b929f777cf734c78a132d0caeea315973 Cr-Commit-Position: refs/heads/master@{#391626} |
