Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
272013002:
Provide skeleton for ChromeVox next. (Closed)
|
Created:
6 years, 7 months ago by David Tseng Modified:
6 years, 7 months ago CC:
chromium-reviews, oshima+watch_chromium.org, plundblad+watch_chromium.org, aboxhall+watch_chromium.org, nkostylev+watch_chromium.org, yuzo+watch_chromium.org, dmazzoni+watch_chromium.org, dtseng+watch_chromium.org, stevenjb+watch_chromium.org, arv+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionProvide skeleton for ChromeVox next.
BUG=372578
NOTRY=true
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=271293
Patch Set 1 #
Total comments: 4
Patch Set 2 : Example of runtime insertion of incognito split (still not done cleaning up) #Patch Set 3 : Pull in classic background page. #Patch Set 4 : #
Total comments: 2
Patch Set 5 : Check port name. #
Total comments: 18
Patch Set 6 : Address comments #Patch Set 7 : #Patch Set 8 : Final changes. #Patch Set 9 : Add owners file #Patch Set 10 : Rebase #
Total comments: 2
Patch Set 11 : Indent #Patch Set 12 : Rebase again. #Messages
Total messages: 40 (0 generated)
https://codereview.chromium.org/272013002/diff/1/chrome/browser/browser_resou... File chrome/browser/browser_resources.grd (right): https://codereview.chromium.org/272013002/diff/1/chrome/browser/browser_resou... chrome/browser/browser_resources.grd:300: <include name="IDR_CHROMEVOX_GUEST_MANIFEST" file="resources\chromeos\chromevox\manifest_guest.json" type="BINDATA" /> Don't you want to use a chromevox next guest manifest? https://codereview.chromium.org/272013002/diff/1/chrome/browser/browser_resou... chrome/browser/browser_resources.grd:302: <if expr="not 'use_chromevox_next' in defs"> This essentially makes ChromeVox Next a compile-time flag, which will make testing very difficult. Can we make it a runtime flag? Have separate manifest definitions for both versions, and use a runtime switch to determine which one to load.
https://codereview.chromium.org/272013002/diff/1/chrome/browser/browser_resou... File chrome/browser/browser_resources.grd (right): https://codereview.chromium.org/272013002/diff/1/chrome/browser/browser_resou... chrome/browser/browser_resources.grd:300: <include name="IDR_CHROMEVOX_GUEST_MANIFEST" file="resources\chromeos\chromevox\manifest_guest.json" type="BINDATA" /> On 2014/05/08 23:29:27, dmazzoni wrote: > Don't you want to use a chromevox next guest manifest? What caused us to need to use a guest manifest? I'd like to avoid duplicating another manifest if possible (and simply point to the original manifest). https://codereview.chromium.org/272013002/diff/1/chrome/browser/browser_resou... chrome/browser/browser_resources.grd:302: <if expr="not 'use_chromevox_next' in defs"> On 2014/05/08 23:29:27, dmazzoni wrote: > This essentially makes ChromeVox Next a compile-time flag, which will make > testing very difficult. Can we make it a runtime flag? Have separate manifest > definitions for both versions, and use a runtime switch to determine which one > to load. I'm mixed on this. I did it this way for the initial cl because it requires no changes in the current path pointing to the ChromeVox sources and it doesn't require separate resource ids for ChromeVox and ChromeVox next manifest paths. It won't be hard to make a new target for ChromeVox next tests right?
On Thu, May 8, 2014 at 4:37 PM, <dtseng@chromium.org> wrote: > Don't you want to use a chromevox next guest manifest? > > What caused us to need to use a guest manifest? I'd like to avoid > duplicating another manifest if possible (and simply point to the > original manifest). Pointing to the same manifest is fine. The difference between the real manifest and guest manifest is just one flag relating to incognito split mode, see this change: https://codereview.chromium.org/141733008 I'm mixed on this. I did it this way for the initial cl because it > requires no changes in the current path pointing to the ChromeVox > sources and it doesn't require separate resource ids for ChromeVox and > ChromeVox next manifest paths. > > It won't be hard to make a new target for ChromeVox next tests right? > Unit testing is easy. What I mean is I want it easy to run ChromeVox next on canary channel. Why not just do that work now? To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On Thu, May 8, 2014 at 4:43 PM, Dominic Mazzoni <dmazzoni@chromium.org>wrote: > On Thu, May 8, 2014 at 4:37 PM, <dtseng@chromium.org> wrote: > >> Don't you want to use a chromevox next guest manifest? >> >> What caused us to need to use a guest manifest? I'd like to avoid >> duplicating another manifest if possible (and simply point to the >> original manifest). > > > Pointing to the same manifest is fine. The difference between the real > manifest and guest manifest is just one flag relating to incognito split > mode, see this change: > > https://codereview.chromium.org/141733008 > > I'm mixed on this. I did it this way for the initial cl because it >> requires no changes in the current path pointing to the ChromeVox >> sources and it doesn't require separate resource ids for ChromeVox and >> ChromeVox next manifest paths. >> >> It won't be hard to make a new target for ChromeVox next tests right? >> > > Unit testing is easy. What I mean is I want it easy to run ChromeVox next > on canary channel. Why not just do that work now? > > Well, we'll have two somewhat duplicated copies of ChromeVox on a Chrome OS image; if that's ok, I can make this change. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On Thu, May 8, 2014 at 4:47 PM, David Tseng <dtseng@chromium.org> wrote: > Well, we'll have two somewhat duplicated copies of ChromeVox on a Chrome > OS image; if that's ok, I can make this change. > How about we only build ChromeVox Next in a canary or dev image? That way we get the benefits of being able to test / demo with prebuild images, but we're not affecting the size of release builds. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Dominic Mazzoni writes: > On Thu, May 8, 2014 at 4:47 PM, David Tseng <dtseng@chromium.org> wrote: > > Well, we'll have two somewhat duplicated copies of ChromeVox on a Chrome OS > image; if that's ok, I can make this change. > > > How about we only build ChromeVox Next in a canary or dev image? That way we > get the benefits of being able to test / demo with prebuild images, but we're > not affecting the size of release builds. the problem with that is that if the size difference makes us bump into a limit, that will only be discovered in actual relese builds. I'm not sure if the images are supposed to diverge that much. In any case, I think it is a bit early to put this on any official builds yet. I do think, however, that we should make it so that the two versions can coexist in the same build if we want to. So, wh not: - Make use_chromevox_next enable chromevox next and make it a runtimne switch. Keep the default of 0. - Make the above flag not affect the inclusion of current chromevox in the build - Later we can add the corresponding compile time flag for chromevox_current to easily reenable it if needed. Thanks, //Peter -- To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
David Tseng writes: > > > > On Thu, May 8, 2014 at 4:43 PM, Dominic Mazzoni <dmazzoni@chromium.org> wrote: > > On Thu, May 8, 2014 at 4:37 PM, <dtseng@chromium.org> wrote: > > Don't you want to use a chromevox next guest manifest? > > What caused us to need to use a guest manifest? I'd like to avoid > duplicating another manifest if possible (and simply point to the > original manifest). > > > Pointing to the same manifest is fine. The difference between the real > manifest and guest manifest is just one flag relating to incognito split > mode, see this change: > PLEASE don't add another duplicate manifest. Can you add a manifest_template.json (to make it clear that this is not the actual manifest that gets loaded). Then a tiny python script that makes the actual manifests from that source with the tiny difference neede for guest mode? We can make this script very simple for now and expand it if we need more (or ask the extensions team for conditions in manifest files? :-]) Thanks, //Peter To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2014/05/09 16:09:33, Peter Lundblad wrote: > David Tseng writes: > > > > > > > > On Thu, May 8, 2014 at 4:43 PM, Dominic Mazzoni <mailto:dmazzoni@chromium.org> > wrote: > > > > On Thu, May 8, 2014 at 4:37 PM, <mailto:dtseng@chromium.org> wrote: > > > > Don't you want to use a chromevox next guest manifest? > > > > What caused us to need to use a guest manifest? I'd like to avoid > > duplicating another manifest if possible (and simply point to the > > original manifest). > > > > > > Pointing to the same manifest is fine. The difference between the real > > manifest and guest manifest is just one flag relating to incognito split > > mode, see this change: > > > > PLEASE don't add another duplicate manifest. Can you add a > manifest_template.json (to make it clear that this is not the actual manifest > that gets loaded). > Then a tiny python script that makes the actual manifests from that source with > the tiny > difference neede for guest mode? We can make this script very simple for now > and > expand it if we need more (or ask the extensions team for conditions in > manifest files? :-]) > > Thanks, > //Peter Ok... To summarize, I am ok with doing the following in this cl: - adding a command line flag that will flip between current ChromeVox and ChromeVox next - in component_loader.cc, I'll load a base manifest for ChromeVox from resources; I'll add an additional resource for ChromeVox next. I'll use the::Add variant that allows you to pass base values and insert the incognito split key/value when we're in guest mode. Note that I'm keeping the use_chromevox_next gyp define; we'll remove this once chromevox next is in a testable state.
dtseng@chromium.org writes: > On 2014/05/09 16:09:33, Peter Lundblad wrote: > > David Tseng writes: > > > > > > > > > > > > On Thu, May 8, 2014 at 4:43 PM, Dominic Mazzoni > <mailto:dmazzoni@chromium.org> > > wrote: > > > > > > On Thu, May 8, 2014 at 4:37 PM, <mailto:dtseng@chromium.org> wrote: > > > > > > Don't you want to use a chromevox next guest manifest? > > > > > > What caused us to need to use a guest manifest? I'd like to > > avoid > > > duplicating another manifest if possible (and simply point to > > the > > > original manifest). > > > > > > > > > Pointing to the same manifest is fine. The difference between the > > real > > > manifest and guest manifest is just one flag relating to incognito > > split > > > mode, see this change: > > > > > > PLEASE don't add another duplicate manifest. Can you add a > > manifest_template.json (to make it clear that this is not the actual > > manifest > > that gets loaded). > > Then a tiny python script that makes the actual manifests from that source > with > > the tiny > > difference neede for guest mode? We can make this script very simple for > > now > > and > > expand it if we need more (or ask the extensions team for conditions in > > manifest files? :-]) > > > Thanks, > > //Peter > > > Ok... > > To summarize, I am ok with doing the following in this cl: > - adding a command line flag that will flip between current ChromeVox and > ChromeVox next > - in component_loader.cc, I'll load a base manifest for ChromeVox from > resources; I'll add an additional resource for ChromeVox next. I'll use > the::Add > variant that allows you to pass base values and insert the incognito split > key/value when we're in guest mode. I proposed that exact solution in the code review that added the extra manifest, but that was shot down there because it was deemed too obscure. You might want to go back and look at that review and perhaps add some people to get a different view. I am fine with the rest of what you say. Thanks, //Peter To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
PTAL. I went ahead and added a command line flag. From discussion offline, we're deferring the work to generate manifests as it's not yet clear if we want to do it at runtime or at compile time (each has its advantages and disadvantages). On Fri, May 9, 2014 at 10:20 AM, <plundblad@chromium.org> wrote: > dtseng@chromium.org writes: > > On 2014/05/09 16:09:33, Peter Lundblad wrote: > > > David Tseng writes: > > > > > > > > > > > > > > > > On Thu, May 8, 2014 at 4:43 PM, Dominic Mazzoni > > <mailto:dmazzoni@chromium.org> > > > wrote: > > > > > > > > On Thu, May 8, 2014 at 4:37 PM, <mailto:dtseng@chromium.org> > wrote: > > > > > > > > Don't you want to use a chromevox next guest manifest? > > > > > > > > What caused us to need to use a guest manifest? I'd like to > > > avoid > > > > duplicating another manifest if possible (and simply point to > > > the > > > > original manifest). > > > > > > > > > > > > Pointing to the same manifest is fine. The difference between the > > > real > > > > manifest and guest manifest is just one flag relating to > incognito > > > split > > > > mode, see this change: > > > > > > > > > PLEASE don't add another duplicate manifest. Can you add a > > > manifest_template.json (to make it clear that this is not the actual > > > manifest > > > that gets loaded). > > > Then a tiny python script that makes the actual manifests from that > source > > with > > > the tiny > > > difference neede for guest mode? We can make this script very simple > for > > > now > > > and > > > expand it if we need more (or ask the extensions team for conditions in > > > manifest files? :-]) > > > > > Thanks, > > > //Peter > > > > > > Ok... > > > > To summarize, I am ok with doing the following in this cl: > > - adding a command line flag that will flip between current ChromeVox and > > ChromeVox next > > - in component_loader.cc, I'll load a base manifest for ChromeVox from > > resources; I'll add an additional resource for ChromeVox next. I'll use > > the::Add > > variant that allows you to pass base values and insert the incognito > split > > key/value when we're in guest mode. > > I proposed that exact solution in the code review that added the extra > manifest, but that was shot down there because it was deemed too obscure. > You might want to go back and look at that review and perhaps > add some people to get a different view. > > I am fine with the rest of what you say. > > Thanks, > //Peter > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
ping. This is ready for another look. I've done the following: - added use_chromevox+next compile-time define. - added enable-chromevox-next runtime flag - added basic skeleton injected (content script) and background page js - added simple key event forwarding from content script to background page - background page has a basic exploration mode for the desktop tree (before we hook up the entire key handling -> user commands logic). - cvox2 is now the namespace for ChromeVox next (as opposed to cvox for ChromeVox classic). - both ChromeVox next and ChromeVox classic build to resources/chromeos/chromevox. ChromeVox next addes ChromeVox classic as a dependency. - chromeVox next loads ChromeVox classic's background page. - all ChromeVox next files copy to resources/chromeos/chromevox/chromevox2
Nice! Taking another look now. On Mon, May 12, 2014 at 11:03 AM, <dtseng@chromium.org> wrote: > ping. This is ready for another look. > > I've done the following: > - added use_chromevox+next compile-time define. > - added enable-chromevox-next runtime flag > - added basic skeleton injected (content script) and background page js > - added simple key event forwarding from content script to background page > - background page has a basic exploration mode for the desktop tree > (before we > hook up the entire key handling -> user commands logic). > - cvox2 is now the namespace for ChromeVox next (as opposed to cvox for > ChromeVox classic). > - both ChromeVox next and ChromeVox classic build to > resources/chromeos/chromevox. ChromeVox next addes ChromeVox classic as a > dependency. > - chromeVox next loads ChromeVox classic's background page. > - all ChromeVox next files copy to resources/chromeos/chromevox/chromevox2 > > > > > https://codereview.chromium.org/272013002/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Looks great. https://codereview.chromium.org/272013002/diff/300001/build/common.gypi File build/common.gypi (right): https://codereview.chromium.org/272013002/diff/300001/build/common.gypi#newco... build/common.gypi:1373: # Whether to use the upcoming version of ChromeVox. Perhaps this should make it more clear that this "builds" ChromeVox Next, it doesn't make Chrome "use" it unless you also enable a flag. Also, note that this is Chrome OS only. How about: # Chrome OS: whether to also build the upcoming version of # ChromeVox, which can then be enabled via a command-line switch. 'build_chromevox_next%': 0, https://codereview.chromium.org/272013002/diff/320001/chrome/browser/resource... File chrome/browser/resources/chromeos/chromevox_next/chromevox2/background/background.js (right): https://codereview.chromium.org/272013002/diff/320001/chrome/browser/resource... chrome/browser/resources/chromeos/chromevox_next/chromevox2/background/background.js:26: chrome.automation.getDesktop(this.onDesktopAvailable); Do you not need to bind this?
lgtm
https://codereview.chromium.org/272013002/diff/320001/chrome/browser/resource... File chrome/browser/resources/chromeos/chromevox_next/chromevox.gyp (right): https://codereview.chromium.org/272013002/diff/320001/chrome/browser/resource... chrome/browser/resources/chromeos/chromevox_next/chromevox.gyp:10: 'target_name': 'chromevox_resources', Please pick a unique target name so it is easier to build this separately on the command line. https://codereview.chromium.org/272013002/diff/320001/chrome/browser/resource... chrome/browser/resources/chromeos/chromevox_next/chromevox.gyp:14: 'destination': '<(PRODUCT_DIR)/resources/chromeos/chromevox/chromevox2/background', Why don't you create chromevox_next under chromeos and get rid of 'chromevox2'? It seems confusing to sometimes call this chromevox2 and sometimes chromevox_next. https://codereview.chromium.org/272013002/diff/320001/chrome/browser/resource... chrome/browser/resources/chromeos/chromevox_next/chromevox.gyp:16: 'chromevox2/background/background.html', Why do we need both a html file and js file for the background page? https://codereview.chromium.org/272013002/diff/320001/chrome/browser/resource... File chrome/browser/resources/chromeos/chromevox_next/chromevox2/background/background.html (right): https://codereview.chromium.org/272013002/diff/320001/chrome/browser/resource... chrome/browser/resources/chromeos/chromevox_next/chromevox2/background/background.html:1: <!-- ChromeVox classic --> License text? https://codereview.chromium.org/272013002/diff/320001/chrome/browser/resource... File chrome/browser/resources/chromeos/chromevox_next/chromevox2/background/background.js (right): https://codereview.chromium.org/272013002/diff/320001/chrome/browser/resource... chrome/browser/resources/chromeos/chromevox_next/chromevox2/background/background.js:6: * @fileoverview The entry point for all ChromeVox2 related code for the Please decide on chromevox_next vs. chromevox2. ;) https://codereview.chromium.org/272013002/diff/320001/chrome/browser/resource... chrome/browser/resources/chromeos/chromevox_next/chromevox2/background/background.js:30: onDesktopAvailable: function(tree) { jsdoc? https://codereview.chromium.org/272013002/diff/320001/chrome/browser/resource... chrome/browser/resources/chromeos/chromevox_next/chromevox2/background/background.js:36: if (port.name != 'chromevox2') Make a constant. https://codereview.chromium.org/272013002/diff/320001/chrome/chrome_resources... File chrome/chrome_resources.gyp (right): https://codereview.chromium.org/272013002/diff/320001/chrome/chrome_resources... chrome/chrome_resources.gyp:115: }], nit: indentation.
https://codereview.chromium.org/272013002/diff/300001/build/common.gypi File build/common.gypi (right): https://codereview.chromium.org/272013002/diff/300001/build/common.gypi#newco... build/common.gypi:1373: # Whether to use the upcoming version of ChromeVox. On 2014/05/12 18:18:12, dmazzoni wrote: > Perhaps this should make it more clear that this "builds" ChromeVox Next, it > doesn't make Chrome "use" it unless you also enable a flag. Also, note that this > is Chrome OS only. How about: > > # Chrome OS: whether to also build the upcoming version of > # ChromeVox, which can then be enabled via a command-line switch. > 'build_chromevox_next%': 0, > Done. https://codereview.chromium.org/272013002/diff/320001/chrome/browser/resource... File chrome/browser/resources/chromeos/chromevox_next/chromevox.gyp (right): https://codereview.chromium.org/272013002/diff/320001/chrome/browser/resource... chrome/browser/resources/chromeos/chromevox_next/chromevox.gyp:10: 'target_name': 'chromevox_resources', On 2014/05/12 18:37:32, Peter Lundblad wrote: > Please pick a unique target name so it is easier to build this separately on the > command line. Done. https://codereview.chromium.org/272013002/diff/320001/chrome/browser/resource... chrome/browser/resources/chromeos/chromevox_next/chromevox.gyp:14: 'destination': '<(PRODUCT_DIR)/resources/chromeos/chromevox/chromevox2/background', On 2014/05/12 18:37:32, Peter Lundblad wrote: > Why don't you create chromevox_next under chromeos and get rid of 'chromevox2'? > It seems confusing to sometimes call this chromevox2 and sometimes > chromevox_next. I created chromeos/chromevox2/chromevox2/* This reflects the directory structure of the output files. (i.e. chromevox2: (the first one) is the target chromevox2/chromevox2: is the root of all files under the cvox2 namespace. Does that make sense? https://codereview.chromium.org/272013002/diff/320001/chrome/browser/resource... chrome/browser/resources/chromeos/chromevox_next/chromevox.gyp:16: 'chromevox2/background/background.html', On 2014/05/12 18:37:32, Peter Lundblad wrote: > Why do we need both a html file and js file for the background page? The html file includes all other dependencies (e.g. ChromeVox classic's background page). https://codereview.chromium.org/272013002/diff/320001/chrome/browser/resource... File chrome/browser/resources/chromeos/chromevox_next/chromevox2/background/background.html (right): https://codereview.chromium.org/272013002/diff/320001/chrome/browser/resource... chrome/browser/resources/chromeos/chromevox_next/chromevox2/background/background.html:1: <!-- ChromeVox classic --> On 2014/05/12 18:37:32, Peter Lundblad wrote: > License text? Other html files under resources don't include one. https://codereview.chromium.org/272013002/diff/320001/chrome/browser/resource... File chrome/browser/resources/chromeos/chromevox_next/chromevox2/background/background.js (right): https://codereview.chromium.org/272013002/diff/320001/chrome/browser/resource... chrome/browser/resources/chromeos/chromevox_next/chromevox2/background/background.js:6: * @fileoverview The entry point for all ChromeVox2 related code for the On 2014/05/12 18:37:32, Peter Lundblad wrote: > Please decide on chromevox_next vs. chromevox2. ;) ChromeVox2 it is then. Note that I didn't change use_chromevox_next and enable_chromevox_next since these flags are simply stating the intent to use the next version of ChromeVox (ChromeVox2 is just a programmatic identifier). I can change this if you feel strongly. https://codereview.chromium.org/272013002/diff/320001/chrome/browser/resource... chrome/browser/resources/chromeos/chromevox_next/chromevox2/background/background.js:26: chrome.automation.getDesktop(this.onDesktopAvailable); On 2014/05/12 18:18:12, dmazzoni wrote: > Do you not need to bind this? Not using 'this' within the function, but done anyway. https://codereview.chromium.org/272013002/diff/320001/chrome/browser/resource... chrome/browser/resources/chromeos/chromevox_next/chromevox2/background/background.js:30: onDesktopAvailable: function(tree) { On 2014/05/12 18:37:32, Peter Lundblad wrote: > jsdoc? We need to decide pretty quickly if we want lots of js doc. The other js files here are very bare bones. I added js doc anyway. https://codereview.chromium.org/272013002/diff/320001/chrome/browser/resource... chrome/browser/resources/chromeos/chromevox_next/chromevox2/background/background.js:36: if (port.name != 'chromevox2') On 2014/05/12 18:37:32, Peter Lundblad wrote: > Make a constant. Done. https://codereview.chromium.org/272013002/diff/320001/chrome/chrome_resources... File chrome/chrome_resources.gyp (right): https://codereview.chromium.org/272013002/diff/320001/chrome/chrome_resources... chrome/chrome_resources.gyp:115: }], On 2014/05/12 18:37:32, Peter Lundblad wrote: > nit: indentation. Done.
dtseng@chromium.org writes: > > https://codereview.chromium.org/272013002/diff/320001/chrome/browser/resource... > chrome/browser/resources/chromeos/chromevox_next/chromevox.gyp:14: > 'destination': > '<(PRODUCT_DIR)/resources/chromeos/chromevox/chromevox2/background', > On 2014/05/12 18:37:32, Peter Lundblad wrote: > > Why don't you create chromevox_next under chromeos and get rid of > 'chromevox2'? > > It seems confusing to sometimes call this chromevox2 and sometimes > > chromevox_next. > > I created chromeos/chromevox2/chromevox2/* > > This reflects the directory structure of the output files. > (i.e. > chromevox2: (the first one) is the target > chromevox2/chromevox2: is the root of all files under the cvox2 > namespace. > > Does that make sense? It's better, but maybe if we want to be consistent with the namespaces, we could allow ourselves to call it cvox2 making it really obvious and straightforward? > https://codereview.chromium.org/272013002/diff/320001/chrome/browser/resource... > chrome/browser/resources/chromeos/chromevox_next/chromevox.gyp:16: > 'chromevox2/background/background.html', > On 2014/05/12 18:37:32, Peter Lundblad wrote: > > Why do we need both a html file and js file for the background page? > > The html file includes all other dependencies (e.g. ChromeVox classic's > background page). I see that, but it is easier to work with for compiling and perhaps flattening (if we want that) if there's one top-level js file. Is there a particular reason to not just have the top-level in js instead of html? > https://codereview.chromium.org/272013002/diff/320001/chrome/browser/resource... > File > chrome/browser/resources/chromeos/chromevox_next/chromevox2/background/background.html > (right): > > https://codereview.chromium.org/272013002/diff/320001/chrome/browser/resource... > chrome/browser/resources/chromeos/chromevox_next/chromevox2/background/background.html:1: > <!-- ChromeVox classic --> > On 2014/05/12 18:37:32, Peter Lundblad wrote: > > License text? > > Other html files under resources don't include one. I see lots of examples of both and I don't know which is correct, so I am fine to leave (unless you get rid of the html as suggested above). > https://codereview.chromium.org/272013002/diff/320001/chrome/browser/resource... > File > chrome/browser/resources/chromeos/chromevox_next/chromevox2/background/background.js > (right): > > https://codereview.chromium.org/272013002/diff/320001/chrome/browser/resource... > chrome/browser/resources/chromeos/chromevox_next/chromevox2/background/background.js:6: > * @fileoverview The entry point for all ChromeVox2 related code for the > On 2014/05/12 18:37:32, Peter Lundblad wrote: > > Please decide on chromevox_next vs. chromevox2. ;) > > ChromeVox2 it is then. > > Note that I didn't change use_chromevox_next and enable_chromevox_next > since these flags are simply stating the intent to use the next version > of ChromeVox (ChromeVox2 is just a programmatic identifier). I can > change this if you feel strongly. Fine with me. > https://codereview.chromium.org/272013002/diff/320001/chrome/browser/resource... > chrome/browser/resources/chromeos/chromevox_next/chromevox2/background/background.js:30: > onDesktopAvailable: function(tree) { > On 2014/05/12 18:37:32, Peter Lundblad wrote: > > jsdoc? > > We need to decide pretty quickly if we want lots of js doc. The other js > files here are very bare bones. I added js doc anyway. Yes, we should decide, perhaps off line. I am in favour of documentation that adds value and is not redundant. If we want to continue using the compiler for static checking, which I'd prefer, then we want to at least keep the annotatons. //Peter To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2014/05/12 20:50:13, Peter Lundblad wrote: > mailto:dtseng@chromium.org writes: > > > > > https://codereview.chromium.org/272013002/diff/320001/chrome/browser/resource... > > chrome/browser/resources/chromeos/chromevox_next/chromevox.gyp:14: > > 'destination': > > '<(PRODUCT_DIR)/resources/chromeos/chromevox/chromevox2/background', > > On 2014/05/12 18:37:32, Peter Lundblad wrote: > > > Why don't you create chromevox_next under chromeos and get rid of > > 'chromevox2'? > > > It seems confusing to sometimes call this chromevox2 and sometimes > > > chromevox_next. > > > > I created chromeos/chromevox2/chromevox2/* > > > > This reflects the directory structure of the output files. > > (i.e. > > chromevox2: (the first one) is the target > > chromevox2/chromevox2: is the root of all files under the cvox2 > > namespace. > > > > Does that make sense? > > It's better, but maybe if we want to be consistent with the namespaces, we > could allow ourselves to call it cvox2 making it really obvious and > straightforward? Sure; seems fine with me; I didn't do that because chromevox current uses the entire name. You should perhaps do the same (ie.. chromevox/cvox/) when you transition files. > > > > https://codereview.chromium.org/272013002/diff/320001/chrome/browser/resource... > > chrome/browser/resources/chromeos/chromevox_next/chromevox.gyp:16: > > 'chromevox2/background/background.html', > > On 2014/05/12 18:37:32, Peter Lundblad wrote: > > > Why do we need both a html file and js file for the background page? > > > > The html file includes all other dependencies (e.g. ChromeVox classic's > > background page). > > I see that, but it is easier to work with for compiling and perhaps flattening > (if we want that) if there's one top-level js file. Is there a particular > reason to > not just have the top-level in js instead of html? For one, we don't have a strategy for flattening yet, so let's call this a todo. We also haven't decided on how we're doing dependency management. I also need the js files to be included in a specific order (ChromeVox classic first, then ChromeVox next). > > > > https://codereview.chromium.org/272013002/diff/320001/chrome/browser/resource... > > File > > > chrome/browser/resources/chromeos/chromevox_next/chromevox2/background/background.html > > (right): > > > > > https://codereview.chromium.org/272013002/diff/320001/chrome/browser/resource... > > > chrome/browser/resources/chromeos/chromevox_next/chromevox2/background/background.html:1: > > <!-- ChromeVox classic --> > > On 2014/05/12 18:37:32, Peter Lundblad wrote: > > > License text? > > > > Other html files under resources don't include one. > > I see lots of examples of both and I don't know which is correct, so I am fine > to leave (unless you get rid of the html as suggested above). > > > > https://codereview.chromium.org/272013002/diff/320001/chrome/browser/resource... > > File > > > chrome/browser/resources/chromeos/chromevox_next/chromevox2/background/background.js > > (right): > > > > > https://codereview.chromium.org/272013002/diff/320001/chrome/browser/resource... > > > chrome/browser/resources/chromeos/chromevox_next/chromevox2/background/background.js:6: > > * @fileoverview The entry point for all ChromeVox2 related code for the > > On 2014/05/12 18:37:32, Peter Lundblad wrote: > > > Please decide on chromevox_next vs. chromevox2. ;) > > > > ChromeVox2 it is then. > > > > Note that I didn't change use_chromevox_next and enable_chromevox_next > > since these flags are simply stating the intent to use the next version > > of ChromeVox (ChromeVox2 is just a programmatic identifier). I can > > change this if you feel strongly. > > Fine with me. > > > > https://codereview.chromium.org/272013002/diff/320001/chrome/browser/resource... > > > chrome/browser/resources/chromeos/chromevox_next/chromevox2/background/background.js:30: > > onDesktopAvailable: function(tree) { > > On 2014/05/12 18:37:32, Peter Lundblad wrote: > > > jsdoc? > > > > We need to decide pretty quickly if we want lots of js doc. The other js > > files here are very bare bones. I added js doc anyway. > > Yes, we should decide, perhaps off line. I am in favour of documentation that > adds value and is not redundant. If we want to continue using the compiler > for static checking, which I'd prefer, then we want to at least keep the > annotatons. > > Ok. > > //Peter > > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:chromium-reviews+unsubscribe@chromium.org.
lgtm
+ darren@ for OWNERS approval for various chrome/ changes. Thanks.
+ darin@
Friendly ping @darin. Let me know your thoughts or if you need further clarification about this one. On Mon, May 12, 2014 at 3:59 PM, <dtseng@chromium.org> wrote: > + darin@ > > > > https://codereview.chromium.org/272013002/ > > To unsubscribe from this group and stop receiving emails from it, send an > email to chromium-reviews+unsubscribe@chromium.org. > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
+thakis Trying another owner. Thanks.
+ jam. Any OWNERS around?
LGTM for the basic structure. I don't have much comment on the actual structure. https://codereview.chromium.org/272013002/diff/410001/chromeos/chromeos_switc... File chromeos/chromeos_switches.cc (right): https://codereview.chromium.org/272013002/diff/410001/chromeos/chromeos_switc... chromeos/chromeos_switches.cc:73: const char kEnableChromeVoxNext[] = "enable-chromevox-next"; nit: I think some effort is made in this file to have the "=" line up in a column. That convention seems poorly implemented. However, you've placed the "=" at a seemingly random location.
https://codereview.chromium.org/272013002/diff/410001/chromeos/chromeos_switc... File chromeos/chromeos_switches.cc (right): https://codereview.chromium.org/272013002/diff/410001/chromeos/chromeos_switc... chromeos/chromeos_switches.cc:73: const char kEnableChromeVoxNext[] = "enable-chromevox-next"; On 2014/05/16 21:54:07, darin wrote: > nit: I think some effort is made in this file to have the "=" line up in a > column. That convention seems poorly implemented. However, you've placed the "=" > at a seemingly random location. Done; thanks.
The CQ bit was checked by dtseng@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dtseng@chromium.org/272013002/430001
FYI, CQ is re-trying this CL (attempt #1). Please consider checking whether the failures are real, and report flakes to chrome-troopers@google.com. The failing builders are: android_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_dbg/builds/18...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_chromium_gn_compile_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_chromium_gn_c...) chromium_presubmit on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...) linux_chromium_chromeos_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...) linux_chromium_clang_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_clang_...) linux_chromium_gn_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_gn_rel...) linux_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...)
The CQ bit was checked by dtseng@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dtseng@chromium.org/272013002/450001
FYI, CQ is re-trying this CL (attempt #1). Please consider checking whether the failures are real, and report flakes to chrome-troopers@google.com. The failing builders are: android_chromium_gn_compile_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_chromium_gn_c...) android_dbg_triggered_tests on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...) chromium_presubmit on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...) linux_chromium_chromeos_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...) linux_chromium_clang_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_clang_...) linux_chromium_gn_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_gn_rel...) linux_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_chromeos_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
The CQ bit was checked by dtseng@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dtseng@chromium.org/272013002/450001
Message was sent while issue was closed.
Change committed as 271293 |
