Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
671333002:
Hotword Audio Verification App: Fix window handling. (Closed)
|
Created:
6 years, 2 months ago by kcarattini Modified:
6 years, 1 month ago CC:
chromium-reviews, arv+watch_chromium.org, chrome-apps-syd-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Project:
chromium Visibility:
Public. |
DescriptionHotword Audio Verification App: Fix window handling.
Minimizes the time of the blank page before before the app UI shows.
Focuses the app rather than opening another window if the app is already
running.
BUG=390086
Committed: https://crrev.com/6fdf39c8fcf627270840b321cf329d53ab7261d1
Cr-Commit-Position: refs/heads/master@{#301993}
Patch Set 1 #Patch Set 2 : Remove debugging code #
Total comments: 3
Patch Set 3 : Review comments #Patch Set 4 : Removed blank line #
Total comments: 5
Patch Set 5 : Review comments #Patch Set 6 : Rebase #Patch Set 7 : Review comments #Patch Set 8 : Rebase #
Total comments: 10
Patch Set 9 : Review comments #Patch Set 10 : Typo #
Messages
Total messages: 21 (3 generated)
kcarattini@chromium.org changed reviewers: + benwells@chromium.org
kcarattini@chromium.org changed reviewers: + dbeam@chromium.org
dbeam: please review for OWNERS approval in chrome/browser/resources/...
https://codereview.chromium.org/671333002/diff/20001/chrome/browser/resources... File chrome/browser/resources/hotword_audio_verification/event_page.js (right): https://codereview.chromium.org/671333002/diff/20001/chrome/browser/resources... chrome/browser/resources/hotword_audio_verification/event_page.js:12: } seems like this should already happen[1]: """ If a window with a given id is created while another window with the same id already exists, the currently opened window will be focused instead of creating a new window. """ [1] https://developer.chrome.com/apps/app_window#type-CreateWindowOptions
https://codereview.chromium.org/671333002/diff/20001/chrome/browser/resources... File chrome/browser/resources/hotword_audio_verification/event_page.js (right): https://codereview.chromium.org/671333002/diff/20001/chrome/browser/resources... chrome/browser/resources/hotword_audio_verification/event_page.js:12: } On 2014/10/24 00:31:48, Dan Beam wrote: > seems like this should already happen[1]: > > """ > If a window with a given id is created while another window with the same id > already exists, the currently opened window will be focused instead of creating > a new window. > """ > > [1] https://developer.chrome.com/apps/app_window#type-CreateWindowOptions When I tested, the window did not focus, that's why I added the code. Not sure why it didn't happen ...
On 2014/10/24 00:35:27, kcarattini wrote: > https://codereview.chromium.org/671333002/diff/20001/chrome/browser/resources... > File chrome/browser/resources/hotword_audio_verification/event_page.js (right): > > https://codereview.chromium.org/671333002/diff/20001/chrome/browser/resources... > chrome/browser/resources/hotword_audio_verification/event_page.js:12: } > On 2014/10/24 00:31:48, Dan Beam wrote: > > seems like this should already happen[1]: > > > > """ > > If a window with a given id is created while another window with the same id > > already exists, the currently opened window will be focused instead of > creating > > a new window. > > """ > > > > [1] https://developer.chrome.com/apps/app_window#type-CreateWindowOptions > > When I tested, the window did not focus, that's why I added the code. Not sure > why it didn't happen ... can you file a bug with a minimal repro case then?
https://codereview.chromium.org/671333002/diff/20001/chrome/browser/resources... File chrome/browser/resources/hotword_audio_verification/event_page.js (right): https://codereview.chromium.org/671333002/diff/20001/chrome/browser/resources... chrome/browser/resources/hotword_audio_verification/event_page.js:12: } On 2014/10/24 00:35:27, kcarattini wrote: > On 2014/10/24 00:31:48, Dan Beam wrote: > > seems like this should already happen[1]: > > > > """ > > If a window with a given id is created while another window with the same id > > already exists, the currently opened window will be focused instead of > creating > > a new window. > > """ > > > > [1] https://developer.chrome.com/apps/app_window#type-CreateWindowOptions > > When I tested, the window did not focus, that's why I added the code. Not sure > why it didn't happen ... Issue https://code.google.com/p/chromium/issues/detail?id=426725 filed and added a TODO to remove this once it's fixed.
https://codereview.chromium.org/671333002/diff/60001/chrome/browser/resources... File chrome/browser/resources/hotword_audio_verification/flow.js (right): https://codereview.chromium.org/671333002/diff/60001/chrome/browser/resources... chrome/browser/resources/hotword_audio_verification/flow.js:85: chrome.app.window.current().show(); can current() return null or undefined?
https://codereview.chromium.org/671333002/diff/60001/chrome/browser/resources... File chrome/browser/resources/hotword_audio_verification/event_page.js (right): https://codereview.chromium.org/671333002/diff/60001/chrome/browser/resources... chrome/browser/resources/hotword_audio_verification/event_page.js:8: // TODO(kcarattini): Remove this when crbug/426725 is fixed. I am guessing the window isn't shown as you've got hidden: true below. If the window exists, instead of doing this you can probably just remove hidden: true from the parameters.
https://codereview.chromium.org/671333002/diff/60001/chrome/browser/resources... File chrome/browser/resources/hotword_audio_verification/event_page.js (right): https://codereview.chromium.org/671333002/diff/60001/chrome/browser/resources... chrome/browser/resources/hotword_audio_verification/event_page.js:8: // TODO(kcarattini): Remove this when crbug/426725 is fixed. On 2014/10/24 04:34:17, benwells wrote: > I am guessing the window isn't shown as you've got hidden: true below. If the > window exists, instead of doing this you can probably just remove hidden: true > from the parameters. Confirmed that the window isn't focused due to the 'hidden' attribute. I've updated the comment.
lgtm
https://codereview.chromium.org/671333002/diff/60001/chrome/browser/resources... File chrome/browser/resources/hotword_audio_verification/flow.js (right): https://codereview.chromium.org/671333002/diff/60001/chrome/browser/resources... chrome/browser/resources/hotword_audio_verification/flow.js:85: chrome.app.window.current().show(); On 2014/10/24 02:37:19, Dan Beam wrote: > can current() return null or undefined? ping
https://codereview.chromium.org/671333002/diff/60001/chrome/browser/resources... File chrome/browser/resources/hotword_audio_verification/flow.js (right): https://codereview.chromium.org/671333002/diff/60001/chrome/browser/resources... chrome/browser/resources/hotword_audio_verification/flow.js:85: chrome.app.window.current().show(); On 2014/10/27 15:53:38, Dan Beam wrote: > On 2014/10/24 02:37:19, Dan Beam wrote: > > can current() return null or undefined? > > ping Done.
https://codereview.chromium.org/671333002/diff/140001/chrome/browser/resource... File chrome/browser/resources/hotword_audio_verification/event_page.js (right): https://codereview.chromium.org/671333002/diff/140001/chrome/browser/resource... chrome/browser/resources/hotword_audio_verification/event_page.js:10: var appWindow = chrome.app.window.get(appId); what if this window is hidden? do we need to unhide? https://codereview.chromium.org/671333002/diff/140001/chrome/browser/resource... chrome/browser/resources/hotword_audio_verification/event_page.js:19: 'hidden': true, why is this created hidden? https://codereview.chromium.org/671333002/diff/140001/chrome/browser/resource... File chrome/browser/resources/hotword_audio_verification/flow.js (right): https://codereview.chromium.org/671333002/diff/140001/chrome/browser/resource... chrome/browser/resources/hotword_audio_verification/flow.js:86: if (appWindow) this doesn't answer my question as I don't know whether it's OK to drop this if there's no window.
https://codereview.chromium.org/671333002/diff/140001/chrome/browser/resource... File chrome/browser/resources/hotword_audio_verification/event_page.js (right): https://codereview.chromium.org/671333002/diff/140001/chrome/browser/resource... chrome/browser/resources/hotword_audio_verification/event_page.js:10: var appWindow = chrome.app.window.get(appId); On 2014/10/27 23:13:20, Dan Beam wrote: > what if this window is hidden? do we need to unhide? The window is always shown very shortly after being created (see my explanation below). https://codereview.chromium.org/671333002/diff/140001/chrome/browser/resource... chrome/browser/resources/hotword_audio_verification/event_page.js:19: 'hidden': true, On 2014/10/27 23:13:20, Dan Beam wrote: > why is this created hidden? I had a bug where the window would flash grey for a second and then show the correct page. I was trying to minimise this by not showing the window until the correct page was marked as not hidden (in flow.js), so as not to show the blank grey page. https://codereview.chromium.org/671333002/diff/140001/chrome/browser/resource... File chrome/browser/resources/hotword_audio_verification/flow.js (right): https://codereview.chromium.org/671333002/diff/140001/chrome/browser/resource... chrome/browser/resources/hotword_audio_verification/flow.js:86: if (appWindow) On 2014/10/27 23:13:20, Dan Beam wrote: > this doesn't answer my question as I don't know whether it's OK to drop this if > there's no window. There should always be an app window in this context.
lgtm https://codereview.chromium.org/671333002/diff/140001/chrome/browser/resource... File chrome/browser/resources/hotword_audio_verification/event_page.js (right): https://codereview.chromium.org/671333002/diff/140001/chrome/browser/resource... chrome/browser/resources/hotword_audio_verification/event_page.js:10: var appWindow = chrome.app.window.get(appId); On 2014/10/27 23:24:12, kcarattini wrote: > On 2014/10/27 23:13:20, Dan Beam wrote: > > what if this window is hidden? do we need to unhide? > > The window is always shown very shortly after being created (see my explanation > below). does this work with window managers that support hiding (e.g. Cmd+h on an app window on Mac)? https://codereview.chromium.org/671333002/diff/140001/chrome/browser/resource... File chrome/browser/resources/hotword_audio_verification/flow.js (right): https://codereview.chromium.org/671333002/diff/140001/chrome/browser/resource... chrome/browser/resources/hotword_audio_verification/flow.js:86: if (appWindow) On 2014/10/27 23:24:12, kcarattini wrote: > On 2014/10/27 23:13:20, Dan Beam wrote: > > this doesn't answer my question as I don't know whether it's OK to drop this > if > > there's no window. > > There should always be an app window in this context. then please leave the code as it was so we can catch unexpected situations
https://codereview.chromium.org/671333002/diff/140001/chrome/browser/resource... File chrome/browser/resources/hotword_audio_verification/event_page.js (right): https://codereview.chromium.org/671333002/diff/140001/chrome/browser/resource... chrome/browser/resources/hotword_audio_verification/event_page.js:10: var appWindow = chrome.app.window.get(appId); On 2014/10/27 23:29:20, Dan Beam wrote: > On 2014/10/27 23:24:12, kcarattini wrote: > > On 2014/10/27 23:13:20, Dan Beam wrote: > > > what if this window is hidden? do we need to unhide? > > > > The window is always shown very shortly after being created (see my > explanation > > below). > > does this work with window managers that support hiding (e.g. Cmd+h on an app > window on Mac)? Added a note, as discussed. https://codereview.chromium.org/671333002/diff/140001/chrome/browser/resource... File chrome/browser/resources/hotword_audio_verification/flow.js (right): https://codereview.chromium.org/671333002/diff/140001/chrome/browser/resource... chrome/browser/resources/hotword_audio_verification/flow.js:86: if (appWindow) On 2014/10/27 23:29:20, Dan Beam wrote: > On 2014/10/27 23:24:12, kcarattini wrote: > > On 2014/10/27 23:13:20, Dan Beam wrote: > > > this doesn't answer my question as I don't know whether it's OK to drop this > > if > > > there's no window. > > > > There should always be an app window in this context. > > then please leave the code as it was so we can catch unexpected situations Done. Sorry, I misunderstood your earlier question.
The CQ bit was checked by kcarattini@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/671333002/180001
Message was sent while issue was closed.
Committed patchset #10 (id:180001)
Message was sent while issue was closed.
Patchset 10 (id:??) landed as https://crrev.com/6fdf39c8fcf627270840b321cf329d53ab7261d1 Cr-Commit-Position: refs/heads/master@{#301993} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
