Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
209313002:
Modified components ui to address concern of all the time disabled check update button. (Closed)
|
Created:
6 years, 9 months ago by Shrikant Kelkar Modified:
6 years, 6 months ago CC:
chromium-reviews, arv+watch_chromium.org, ericde Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionModified components ui to address concern of all the time disabled check update button.
With this change we take into account current status of update from update service.
+jhawkins for owners
BUG=272540
R=sorin,cpu,jhawkins
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=274385
Patch Set 1 #
Total comments: 12
Patch Set 2 : Modified as per code review comments. #
Total comments: 2
Patch Set 3 : Removed comment as mentioned in code review. #
Total comments: 22
Patch Set 4 : Code review changes. #
Total comments: 11
Patch Set 5 : Code review #Patch Set 6 : Added back status text. #Patch Set 7 : Changes in sync with new Observer interface. #Patch Set 8 : Latest sync #
Total comments: 8
Patch Set 9 : Modifications to expose CrxUpdateItem to ComponentsUI as per discussion. #
Total comments: 6
Patch Set 10 : Addressing code review comments. #Patch Set 11 : Modified according to code review comments. #
Total comments: 16
Patch Set 12 : Code review update. #Patch Set 13 : Code review #
Total comments: 6
Patch Set 14 : Code review. #
Total comments: 12
Patch Set 15 : Manual merge with TOT #Patch Set 16 : Code review #
Total comments: 9
Patch Set 17 : Code review #
Total comments: 15
Patch Set 18 : Code review. #
Total comments: 2
Patch Set 19 : Code review. #Patch Set 20 : Solving compiler issues of non handled switch case. #Patch Set 21 : #Messages
Total messages: 64 (0 generated)
looking good, can you post a screenshot? sorin@ can you review?
On 2014/03/25 18:30:50, cpu wrote: > looking good, can you post a screenshot? > > sorin@ can you review? cpu@ attached screenshot to associated bug, ptal. (https://code.google.com/p/chromium/issues/detail?id=272540)
lgtm
Thank you Shrikant. One issue which could be a bug, and a bunch of some other fluff to consider. Thank you! https://codereview.chromium.org/209313002/diff/1/chrome/browser/component_upd... File chrome/browser/component_updater/component_updater_service.cc (right): https://codereview.chromium.org/209313002/diff/1/chrome/browser/component_upd... chrome/browser/component_updater/component_updater_service.cc:518: Status service_status = GetConciseStatus(uit->status); Consider renaming GetConciseStatus to GetServiceStatus. https://codereview.chromium.org/209313002/diff/1/chrome/browser/component_upd... chrome/browser/component_updater/component_updater_service.cc:526: ChangeItemState(uit, CrxUpdateItem::kNew); are we still setting uit->on_demand = true anywhere? https://codereview.chromium.org/209313002/diff/1/chrome/browser/component_upd... chrome/browser/component_updater/component_updater_service.cc:559: DCHECK(crx != NULL); no need to dcheck and handle the error, see the DCHECK section http://www.chromium.org/developers/coding-style I know we have places like this in the code, which we should fix. https://codereview.chromium.org/209313002/diff/1/chrome/browser/component_upd... chrome/browser/component_updater/component_updater_service.cc:564: return service_status; return GetConciseStatus(crx->status); https://codereview.chromium.org/209313002/diff/1/chrome/browser/component_upd... chrome/browser/component_updater/component_updater_service.cc:984: ComponentUpdateService::Status CrxUpdateService::GetConciseStatus( If we don't fully qualify then the param might fit on one line. https://codereview.chromium.org/209313002/diff/1/chrome/browser/component_upd... File chrome/browser/component_updater/component_updater_service.h (right): https://codereview.chromium.org/209313002/diff/1/chrome/browser/component_upd... chrome/browser/component_updater/component_updater_service.h:198: virtual ComponentUpdateService::Status GetComponentStatus( do we need to fully qualify ComponentUpdateService::Status? The other members don't.
ptal. https://codereview.chromium.org/209313002/diff/1/chrome/browser/component_upd... File chrome/browser/component_updater/component_updater_service.cc (right): https://codereview.chromium.org/209313002/diff/1/chrome/browser/component_upd... chrome/browser/component_updater/component_updater_service.cc:518: Status service_status = GetConciseStatus(uit->status); On 2014/03/26 20:22:36, Sorin Jianu wrote: > Consider renaming GetConciseStatus to GetServiceStatus. Done. https://codereview.chromium.org/209313002/diff/1/chrome/browser/component_upd... chrome/browser/component_updater/component_updater_service.cc:526: ChangeItemState(uit, CrxUpdateItem::kNew); On 2014/03/26 20:22:36, Sorin Jianu wrote: > are we still setting uit->on_demand = true anywhere? good catch, copy~paste error. Thanks. https://codereview.chromium.org/209313002/diff/1/chrome/browser/component_upd... chrome/browser/component_updater/component_updater_service.cc:559: DCHECK(crx != NULL); On 2014/03/26 20:22:36, Sorin Jianu wrote: > no need to dcheck and handle the error, see the DCHECK section > http://www.chromium.org/developers/coding-style > > I know we have places like this in the code, which we should fix. Done. https://codereview.chromium.org/209313002/diff/1/chrome/browser/component_upd... chrome/browser/component_updater/component_updater_service.cc:564: return service_status; On 2014/03/26 20:22:36, Sorin Jianu wrote: > return GetConciseStatus(crx->status); Done. https://codereview.chromium.org/209313002/diff/1/chrome/browser/component_upd... chrome/browser/component_updater/component_updater_service.cc:984: ComponentUpdateService::Status CrxUpdateService::GetConciseStatus( On 2014/03/26 20:22:36, Sorin Jianu wrote: > If we don't fully qualify then the param might fit on one line. Not qualifying gives compile error + we seem to be doing it for other functions too (ComponentUpdateService::Status CrxUpdateService::RegisterComponent) https://codereview.chromium.org/209313002/diff/1/chrome/browser/component_upd... File chrome/browser/component_updater/component_updater_service.h (right): https://codereview.chromium.org/209313002/diff/1/chrome/browser/component_upd... chrome/browser/component_updater/component_updater_service.h:198: virtual ComponentUpdateService::Status GetComponentStatus( On 2014/03/26 20:22:36, Sorin Jianu wrote: > do we need to fully qualify ComponentUpdateService::Status? The other members > don't. Done.
lgtm Thank you! https://codereview.chromium.org/209313002/diff/40001/chrome/browser/component... File chrome/browser/component_updater/component_updater_service.cc (right): https://codereview.chromium.org/209313002/diff/40001/chrome/browser/component... chrome/browser/component_updater/component_updater_service.cc:981: // If the item is already in the process of being updated, there is The comments in this function don't make sense anymore. We can remove them.
https://codereview.chromium.org/209313002/diff/40001/chrome/browser/component... File chrome/browser/component_updater/component_updater_service.cc (right): https://codereview.chromium.org/209313002/diff/40001/chrome/browser/component... chrome/browser/component_updater/component_updater_service.cc:981: // If the item is already in the process of being updated, there is On 2014/03/26 21:41:08, Sorin Jianu wrote: > The comments in this function don't make sense anymore. We can remove them. Done.
ping jhawkins@
https://codereview.chromium.org/209313002/diff/60001/chrome/browser/resources... File chrome/browser/resources/components.css (right): https://codereview.chromium.org/209313002/diff/60001/chrome/browser/resources... chrome/browser/resources/components.css:266: #componentTemplate { nit: IDs must be dash-form. https://codereview.chromium.org/209313002/diff/60001/chrome/browser/resources... chrome/browser/resources/components.css:267: visibility: hidden; You should be using the hidden attribute on the element. https://codereview.chromium.org/209313002/diff/60001/chrome/browser/resources... File chrome/browser/resources/components.html (right): https://codereview.chromium.org/209313002/diff/60001/chrome/browser/resources... chrome/browser/resources/components.html:16: <div id="componentPlaceholder"></div> IDs should be dash-form. https://codereview.chromium.org/209313002/diff/60001/chrome/browser/resources... chrome/browser/resources/components.html:54: jscontent="name">NAME</span> nit: Indentation should be 4 spaces in only. https://codereview.chromium.org/209313002/diff/60001/chrome/browser/resources... chrome/browser/resources/components.html:54: jscontent="name">NAME</span> nit: Remove stub content, i.e., NAME. https://codereview.chromium.org/209313002/diff/60001/chrome/browser/resources... chrome/browser/resources/components.html:69: jscontent="status" jsvalues=".id: 'status-' + id">Okay</span> nit: 80 cols. https://codereview.chromium.org/209313002/diff/60001/chrome/browser/resources... File chrome/browser/resources/components.js (right): https://codereview.chromium.org/209313002/diff/60001/chrome/browser/resources... chrome/browser/resources/components.js:17: var output = $('componentTemplate').cloneNode(true); Why do you need to clone? https://codereview.chromium.org/209313002/diff/60001/chrome/browser/resources... chrome/browser/resources/components.js:78: function returnComponentStatus(component_id, status) { Who calls this? https://codereview.chromium.org/209313002/diff/60001/chrome/browser/resources... chrome/browser/resources/components.js:79: $('status-' + component_id).innerText = status; Use textContent, not innerText. https://codereview.chromium.org/209313002/diff/60001/chrome/browser/resources... chrome/browser/resources/components.js:83: window.setTimeout(function() { updateComponentStatus(node); }, 1000); nit: Move 1000 to a well-named constant. https://codereview.chromium.org/209313002/diff/60001/chrome/browser/resources... chrome/browser/resources/components.js:101: $('status-' + String(node.id)).innerText = 'Checking for update...'; textContent https://codereview.chromium.org/209313002/diff/60001/chrome/browser/ui/webui/... File chrome/browser/ui/webui/components_ui.cc (right): https://codereview.chromium.org/209313002/diff/60001/chrome/browser/ui/webui/... chrome/browser/ui/webui/components_ui.cc:99: web_ui()->RegisterMessageCallback("requestComponentStatus", nit: The start of parameter rows must align on the same column.
ptal. https://codereview.chromium.org/209313002/diff/60001/chrome/browser/resources... File chrome/browser/resources/components.html (right): https://codereview.chromium.org/209313002/diff/60001/chrome/browser/resources... chrome/browser/resources/components.html:16: <div id="componentPlaceholder"></div> On 2014/03/27 18:46:43, James Hawkins wrote: > IDs should be dash-form. Done. https://codereview.chromium.org/209313002/diff/60001/chrome/browser/resources... chrome/browser/resources/components.html:54: jscontent="name">NAME</span> On 2014/03/27 18:46:43, James Hawkins wrote: > nit: Indentation should be 4 spaces in only. Done. https://codereview.chromium.org/209313002/diff/60001/chrome/browser/resources... chrome/browser/resources/components.html:54: jscontent="name">NAME</span> On 2014/03/27 18:46:43, James Hawkins wrote: > nit: Remove stub content, i.e., NAME. Done. https://codereview.chromium.org/209313002/diff/60001/chrome/browser/resources... chrome/browser/resources/components.html:69: jscontent="status" jsvalues=".id: 'status-' + id">Okay</span> On 2014/03/27 18:46:43, James Hawkins wrote: > nit: 80 cols. Done. https://codereview.chromium.org/209313002/diff/60001/chrome/browser/resources... File chrome/browser/resources/components.js (right): https://codereview.chromium.org/209313002/diff/60001/chrome/browser/resources... chrome/browser/resources/components.js:17: var output = $('componentTemplate').cloneNode(true); On 2014/03/27 18:46:43, James Hawkins wrote: > Why do you need to clone? I am rewriting contents of the template in jstProcess() https://codereview.chromium.org/209313002/diff/60001/chrome/browser/resources... chrome/browser/resources/components.js:78: function returnComponentStatus(component_id, status) { On 2014/03/27 18:46:43, James Hawkins wrote: > Who calls this? component_ui.cc in response to requestComponentStatus(). https://codereview.chromium.org/209313002/diff/60001/chrome/browser/resources... chrome/browser/resources/components.js:79: $('status-' + component_id).innerText = status; On 2014/03/27 18:46:43, James Hawkins wrote: > Use textContent, not innerText. Done. https://codereview.chromium.org/209313002/diff/60001/chrome/browser/resources... chrome/browser/resources/components.js:83: window.setTimeout(function() { updateComponentStatus(node); }, 1000); On 2014/03/27 18:46:43, James Hawkins wrote: > nit: Move 1000 to a well-named constant. Done. https://codereview.chromium.org/209313002/diff/60001/chrome/browser/resources... chrome/browser/resources/components.js:101: $('status-' + String(node.id)).innerText = 'Checking for update...'; On 2014/03/27 18:46:43, James Hawkins wrote: > textContent Done. https://codereview.chromium.org/209313002/diff/60001/chrome/browser/ui/webui/... File chrome/browser/ui/webui/components_ui.cc (right): https://codereview.chromium.org/209313002/diff/60001/chrome/browser/ui/webui/... chrome/browser/ui/webui/components_ui.cc:99: web_ui()->RegisterMessageCallback("requestComponentStatus", On 2014/03/27 18:46:43, James Hawkins wrote: > nit: The start of parameter rows must align on the same column. Sorry, didn't understand?
On 2014/03/27 19:28:07, Shrikant Kelkar wrote: > ptal. > > https://codereview.chromium.org/209313002/diff/60001/chrome/browser/resources... > File chrome/browser/resources/components.html (right): > > https://codereview.chromium.org/209313002/diff/60001/chrome/browser/resources... > chrome/browser/resources/components.html:16: <div > id="componentPlaceholder"></div> > On 2014/03/27 18:46:43, James Hawkins wrote: > > IDs should be dash-form. > > Done. > > https://codereview.chromium.org/209313002/diff/60001/chrome/browser/resources... > chrome/browser/resources/components.html:54: jscontent="name">NAME</span> > On 2014/03/27 18:46:43, James Hawkins wrote: > > nit: Indentation should be 4 spaces in only. > > Done. > > https://codereview.chromium.org/209313002/diff/60001/chrome/browser/resources... > chrome/browser/resources/components.html:54: jscontent="name">NAME</span> > On 2014/03/27 18:46:43, James Hawkins wrote: > > nit: Remove stub content, i.e., NAME. > > Done. > > https://codereview.chromium.org/209313002/diff/60001/chrome/browser/resources... > chrome/browser/resources/components.html:69: jscontent="status" jsvalues=".id: > 'status-' + id">Okay</span> > On 2014/03/27 18:46:43, James Hawkins wrote: > > nit: 80 cols. > > Done. > > https://codereview.chromium.org/209313002/diff/60001/chrome/browser/resources... > File chrome/browser/resources/components.js (right): > > https://codereview.chromium.org/209313002/diff/60001/chrome/browser/resources... > chrome/browser/resources/components.js:17: var output = > $('componentTemplate').cloneNode(true); > On 2014/03/27 18:46:43, James Hawkins wrote: > > Why do you need to clone? > > I am rewriting contents of the template in jstProcess() > > https://codereview.chromium.org/209313002/diff/60001/chrome/browser/resources... > chrome/browser/resources/components.js:78: function > returnComponentStatus(component_id, status) { > On 2014/03/27 18:46:43, James Hawkins wrote: > > Who calls this? > > component_ui.cc in response to requestComponentStatus(). > > https://codereview.chromium.org/209313002/diff/60001/chrome/browser/resources... > chrome/browser/resources/components.js:79: $('status-' + component_id).innerText > = status; > On 2014/03/27 18:46:43, James Hawkins wrote: > > Use textContent, not innerText. > > Done. > > https://codereview.chromium.org/209313002/diff/60001/chrome/browser/resources... > chrome/browser/resources/components.js:83: window.setTimeout(function() { > updateComponentStatus(node); }, 1000); > On 2014/03/27 18:46:43, James Hawkins wrote: > > nit: Move 1000 to a well-named constant. > > Done. > > https://codereview.chromium.org/209313002/diff/60001/chrome/browser/resources... > chrome/browser/resources/components.js:101: $('status-' + > String(node.id)).innerText = 'Checking for update...'; > On 2014/03/27 18:46:43, James Hawkins wrote: > > textContent > > Done. > > https://codereview.chromium.org/209313002/diff/60001/chrome/browser/ui/webui/... > File chrome/browser/ui/webui/components_ui.cc (right): > > https://codereview.chromium.org/209313002/diff/60001/chrome/browser/ui/webui/... > chrome/browser/ui/webui/components_ui.cc:99: > web_ui()->RegisterMessageCallback("requestComponentStatus", > On 2014/03/27 18:46:43, James Hawkins wrote: > > nit: The start of parameter rows must align on the same column. > > Sorry, didn't understand? The first character of each parameter line (either one parameter per line or multiple parameters on a line) must line up vertically.
https://codereview.chromium.org/209313002/diff/80001/chrome/browser/resources... File chrome/browser/resources/components.html (right): https://codereview.chromium.org/209313002/diff/80001/chrome/browser/resources... chrome/browser/resources/components.html:54: jscontent="name"></span> nit: Closing brace of wrapped line must be on a new line. https://codereview.chromium.org/209313002/diff/80001/chrome/browser/resources... chrome/browser/resources/components.html:56: - <span i18n-content="componentVersion">VERSION</span> Remove placeholder value, VERSION. https://codereview.chromium.org/209313002/diff/80001/chrome/browser/resources... chrome/browser/resources/components.html:57: <span dir="ltr" jscontent="version">x.x.x.x</span> Remove placeholder value, x.x.x.x. https://codereview.chromium.org/209313002/diff/80001/chrome/browser/resources... chrome/browser/resources/components.html:78: jsvalues=".id:id" Indentation should be 4 spaces. https://codereview.chromium.org/209313002/diff/80001/chrome/browser/resources... File chrome/browser/resources/components.js (right): https://codereview.chromium.org/209313002/diff/80001/chrome/browser/resources... chrome/browser/resources/components.js:89: STATUS_REFRESH_INTERVAL); Why are we using polling instead of having the browser update the UI when it changes?
https://codereview.chromium.org/209313002/diff/80001/chrome/browser/resources... File chrome/browser/resources/components.html (right): https://codereview.chromium.org/209313002/diff/80001/chrome/browser/resources... chrome/browser/resources/components.html:54: jscontent="name"></span> On 2014/03/27 20:12:03, James Hawkins wrote: > nit: Closing brace of wrapped line must be on a new line. Done. https://codereview.chromium.org/209313002/diff/80001/chrome/browser/resources... chrome/browser/resources/components.html:56: - <span i18n-content="componentVersion">VERSION</span> On 2014/03/27 20:12:03, James Hawkins wrote: > Remove placeholder value, VERSION. Done. https://codereview.chromium.org/209313002/diff/80001/chrome/browser/resources... chrome/browser/resources/components.html:57: <span dir="ltr" jscontent="version">x.x.x.x</span> On 2014/03/27 20:12:03, James Hawkins wrote: > Remove placeholder value, x.x.x.x. Done. https://codereview.chromium.org/209313002/diff/80001/chrome/browser/resources... chrome/browser/resources/components.html:78: jsvalues=".id:id" On 2014/03/27 20:12:03, James Hawkins wrote: > Indentation should be 4 spaces. Done. https://codereview.chromium.org/209313002/diff/80001/chrome/browser/resources... File chrome/browser/resources/components.js (right): https://codereview.chromium.org/209313002/diff/80001/chrome/browser/resources... chrome/browser/resources/components.js:89: STATUS_REFRESH_INTERVAL); On 2014/03/27 20:12:03, James Hawkins wrote: > Why are we using polling instead of having the browser update the UI when it > changes? The c++ code doesn't have a callback so far.
https://codereview.chromium.org/209313002/diff/80001/chrome/browser/resources... File chrome/browser/resources/components.js (right): https://codereview.chromium.org/209313002/diff/80001/chrome/browser/resources... chrome/browser/resources/components.js:89: STATUS_REFRESH_INTERVAL); On 2014/03/27 22:00:13, Shrikant Kelkar wrote: > On 2014/03/27 20:12:03, James Hawkins wrote: > > Why are we using polling instead of having the browser update the UI when it > > changes? > > The c++ code doesn't have a callback so far. So let's fix it. We shouldn't be polling from the WebUI.
On 2014/03/27 22:02:44, James Hawkins wrote: > https://codereview.chromium.org/209313002/diff/80001/chrome/browser/resources... > File chrome/browser/resources/components.js (right): > > https://codereview.chromium.org/209313002/diff/80001/chrome/browser/resources... > chrome/browser/resources/components.js:89: STATUS_REFRESH_INTERVAL); > On 2014/03/27 22:00:13, Shrikant Kelkar wrote: > > On 2014/03/27 20:12:03, James Hawkins wrote: > > > Why are we using polling instead of having the browser update the UI when it > > > changes? > > > > The c++ code doesn't have a callback so far. > > So let's fix it. We shouldn't be polling from the WebUI. That's fairly big change AFAIK currently component service doesn't allow general observers and it's in polling mode. Wondering what problems we might face if we do polling from WebUI?
On 2014/03/27 22:15:23, Shrikant Kelkar wrote: > On 2014/03/27 22:02:44, James Hawkins wrote: > > > https://codereview.chromium.org/209313002/diff/80001/chrome/browser/resources... > > File chrome/browser/resources/components.js (right): > > > > > https://codereview.chromium.org/209313002/diff/80001/chrome/browser/resources... > > chrome/browser/resources/components.js:89: STATUS_REFRESH_INTERVAL); > > On 2014/03/27 22:00:13, Shrikant Kelkar wrote: > > > On 2014/03/27 20:12:03, James Hawkins wrote: > > > > Why are we using polling instead of having the browser update the UI when > it > > > > changes? > > > > > > The c++ code doesn't have a callback so far. > > > > So let's fix it. We shouldn't be polling from the WebUI. > > That's fairly big change AFAIK currently component service doesn't allow general > observers and it's in polling mode. Wondering what problems we might face if we > do polling from WebUI? It's unnecessarily expensive. Who are the users of this WebUI page?
It's mostly for troubleshooting purposes as of now, but there are future plans. On Thu, Mar 27, 2014 at 3:17 PM, <jhawkins@chromium.org> wrote: > On 2014/03/27 22:15:23, Shrikant Kelkar wrote: > >> On 2014/03/27 22:02:44, James Hawkins wrote: >> > >> > > https://codereview.chromium.org/209313002/diff/80001/ > chrome/browser/resources/components.js > >> > File chrome/browser/resources/components.js (right): >> > >> > >> > > https://codereview.chromium.org/209313002/diff/80001/ > chrome/browser/resources/components.js#newcode89 > >> > chrome/browser/resources/components.js:89: STATUS_REFRESH_INTERVAL); >> > On 2014/03/27 22:00:13, Shrikant Kelkar wrote: >> > > On 2014/03/27 20:12:03, James Hawkins wrote: >> > > > Why are we using polling instead of having the browser update the UI >> > when > >> it >> > > > changes? >> > > >> > > The c++ code doesn't have a callback so far. >> > >> > So let's fix it. We shouldn't be polling from the WebUI. >> > > That's fairly big change AFAIK currently component service doesn't allow >> > general > >> observers and it's in polling mode. Wondering what problems we might face >> if >> > we > >> do polling from WebUI? >> > > It's unnecessarily expensive. Who are the users of this WebUI page? > > https://codereview.chromium.org/209313002/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2014/03/27 22:19:14, shrikant_google.com wrote: > It's mostly for troubleshooting purposes as of now, but there are future > plans. > OK. What is the timeline for getting this particular change in?
No timeline, if are strongly against having UI do the timer, I can certainly abandon this change. This was just convenient thing that we wanted for some time. Wondering if we are going to deprecate setTimeout equivalents for WebUI sometime in future? On Thu, Mar 27, 2014 at 3:20 PM, <jhawkins@chromium.org> wrote: > On 2014/03/27 22:19:14, shrikant_google.com wrote: > >> It's mostly for troubleshooting purposes as of now, but there are future >> plans. >> > > > OK. What is the timeline for getting this particular change in? > > https://codereview.chromium.org/209313002/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2014/03/27 22:26:09, shrikant_google.com wrote: > No timeline, if are strongly against having UI do the timer, I can > certainly abandon this change. This was just convenient thing that we > wanted for some time. Yeah, let's start with the right solution. > Wondering if we are going to deprecate setTimeout equivalents for WebUI > sometime in future? > There's nothing wrong with setTimeout, but we shouldn't be polling if we can avoid it. > > > On Thu, Mar 27, 2014 at 3:20 PM, <mailto:jhawkins@chromium.org> wrote: > > > On 2014/03/27 22:19:14, http://shrikant_google.com wrote: > > > >> It's mostly for troubleshooting purposes as of now, but there are future > >> plans. > >> > > > > > > OK. What is the timeline for getting this particular change in? > > > > https://codereview.chromium.org/209313002/ > > > > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:chromium-reviews+unsubscribe@chromium.org.
If setTimeout is not that wrong then would you consider this as temporary solution till we plugin the whole observer thing from C++ code? We can certainly put TODO, but getting this in definitely will satisfy current requirement. On Thu, Mar 27, 2014 at 3:27 PM, <jhawkins@chromium.org> wrote: > On 2014/03/27 22:26:09, shrikant_google.com wrote: > >> No timeline, if are strongly against having UI do the timer, I can >> certainly abandon this change. This was just convenient thing that we >> wanted for some time. >> > > Yeah, let's start with the right solution. > > > Wondering if we are going to deprecate setTimeout equivalents for WebUI >> sometime in future? >> > > > There's nothing wrong with setTimeout, but we shouldn't be polling if we > can > avoid it. > > > > On Thu, Mar 27, 2014 at 3:20 PM, <mailto:jhawkins@chromium.org> wrote: >> > > > On 2014/03/27 22:19:14, http://shrikant_google.com wrote: >> > >> >> It's mostly for troubleshooting purposes as of now, but there are >> future >> >> plans. >> >> >> > >> > >> > OK. What is the timeline for getting this particular change in? >> > >> > https://codereview.chromium.org/209313002/ >> > >> > > To unsubscribe from this group and stop receiving emails from it, send an >> > email > >> to mailto:chromium-reviews+unsubscribe@chromium.org. >> > > > > https://codereview.chromium.org/209313002/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2014/03/27 22:29:56, shrikant_google.com wrote: > If setTimeout is not that wrong then would you consider this as temporary > solution till we plugin the whole observer thing from C++ code? > We can certainly put TODO, but getting this in definitely will satisfy > current requirement. > setTimeout is fine, just not for polling if it's avoidable, which is what this CL is doing.
James, I wonder if we could reconsider the decision to not poll. I agree that polling is suboptimal in general. However, this chrome://components page is primarily a developer aid. We expect very few views for this page, mostly from the component teams (pNacl, CDM), and possibly for users, when debugging component issues as in "my encrypted videos don't play.". Does that sound reasonable?
On 2014/04/14 15:55:16, Sorin Jianu wrote: > James, I wonder if we could reconsider the decision to not poll. I agree that > polling is suboptimal in general. However, this chrome://components page is > primarily a developer aid. We expect very few views for this page, mostly from > the component teams (pNacl, CDM), and possibly for users, when debugging > component issues as in "my encrypted videos don't play.". Does that sound > reasonable? So did you attempt to do the refactoring? What was the result?
On 2014/04/16 15:32:15, James Hawkins wrote: > On 2014/04/14 15:55:16, Sorin Jianu wrote: > > James, I wonder if we could reconsider the decision to not poll. I agree that > > polling is suboptimal in general. However, this chrome://components page is > > primarily a developer aid. We expect very few views for this page, mostly from > > the component teams (pNacl, CDM), and possibly for users, when debugging > > component issues as in "my encrypted videos don't play.". Does that sound > > reasonable? > > So did you attempt to do the refactoring? What was the result? We have not done the refactoring yet. It will break the existing unit tests and it will be throw away code since we are thinking of merging the extension and component updaters. We agreed to implement the event driven observer, I feel responsible here as I had missed for two weeks the fact there was push back on the CL and we did not land it.
On 2014/04/16 16:00:17, Sorin Jianu wrote: > On 2014/04/16 15:32:15, James Hawkins wrote: > > On 2014/04/14 15:55:16, Sorin Jianu wrote: > > > James, I wonder if we could reconsider the decision to not poll. I agree > that > > > polling is suboptimal in general. However, this chrome://components page is > > > primarily a developer aid. We expect very few views for this page, mostly > from > > > the component teams (pNacl, CDM), and possibly for users, when debugging > > > component issues as in "my encrypted videos don't play.". Does that sound > > > reasonable? > > > > So did you attempt to do the refactoring? What was the result? > > We have not done the refactoring yet. It will break the existing unit tests and > it will be throw away code since we are thinking of merging the extension and > component updaters. We agreed to implement the event driven observer, I feel > responsible here as I had missed for two weeks the fact there was push back on > the CL and we did not land it. OK. If that's the case, why do we need to land this CL?
On 2014/04/16 16:01:28, James Hawkins wrote: > On 2014/04/16 16:00:17, Sorin Jianu wrote: > > On 2014/04/16 15:32:15, James Hawkins wrote: > > > On 2014/04/14 15:55:16, Sorin Jianu wrote: > > > > James, I wonder if we could reconsider the decision to not poll. I agree > > that > > > > polling is suboptimal in general. However, this chrome://components page > is > > > > primarily a developer aid. We expect very few views for this page, mostly > > from > > > > the component teams (pNacl, CDM), and possibly for users, when debugging > > > > component issues as in "my encrypted videos don't play.". Does that sound > > > > reasonable? > > > > > > So did you attempt to do the refactoring? What was the result? > > > > We have not done the refactoring yet. It will break the existing unit tests > and > > it will be throw away code since we are thinking of merging the extension and > > component updaters. We agreed to implement the event driven observer, I feel > > responsible here as I had missed for two weeks the fact there was push back on > > the CL and we did not land it. > > OK. If that's the case, why do we need to land this CL? This CL is improving the chrome://components page so that the state shown on the page is consistent with the state of the updater. Anyway, I will do the event driven observer, thank you for the feedback so far.
Sorin/cpu, ptal. Modified with observer changes.
Thank you Shrikant. I have new changes coming up for the download progress. We could land this and then take other changes for the download progress, either way. https://codereview.chromium.org/209313002/diff/160001/chrome/browser/componen... File chrome/browser/component_updater/component_updater_service.cc (right): https://codereview.chromium.org/209313002/diff/160001/chrome/browser/componen... chrome/browser/component_updater/component_updater_service.cc:522: Status service_status = GetServiceStatus(uit->status); const? https://codereview.chromium.org/209313002/diff/160001/chrome/browser/componen... chrome/browser/component_updater/component_updater_service.cc:563: CrxUpdateItem* crx = FindUpdateItemById(component_id); const ? https://codereview.chromium.org/209313002/diff/160001/chrome/browser/componen... chrome/browser/component_updater/component_updater_service.cc:567: extra line. https://codereview.chromium.org/209313002/diff/160001/chrome/browser/componen... File chrome/browser/component_updater/component_updater_service.h (right): https://codereview.chromium.org/209313002/diff/160001/chrome/browser/componen... chrome/browser/component_updater/component_updater_service.h:235: // Convenience structure to use with component listing / enumeration. For the future, as we want to expose more state information, this structure either will have to include all the state information or we might need to make CrxUpdateItem available to the page. The page is already a friend of the service, so extending the encapsulation boundary in that direction is theoretically possible. I suggest you talk to cpu@ about it. We are not fond of duplicating members from CrxUpdateItem here nor about the friendship. But one way or another we want to make the state information available to the page so developers understand what their component is doing (pretty much, most of the members of the CrxUpdateItem are useful from a dev standpoint when they are looking for answer why their updates are not working). https://codereview.chromium.org/209313002/diff/160001/chrome/browser/componen... chrome/browser/component_updater/component_updater_service.h:244: ~CrxComponentInfo(); maybe dtor not needed? https://codereview.chromium.org/209313002/diff/160001/chrome/browser/resource... File chrome/browser/resources/components.js (right): https://codereview.chromium.org/209313002/diff/160001/chrome/browser/resource... chrome/browser/resources/components.js:79: * This is a fallback function, which handles the case when Do we have to do have the fallback function at all and do the timeout tricks? https://codereview.chromium.org/209313002/diff/160001/chrome/browser/ui/webui... File chrome/browser/ui/webui/components_ui.cc (right): https://codereview.chromium.org/209313002/diff/160001/chrome/browser/ui/webui... chrome/browser/ui/webui/components_ui.cc:213: cus->AddObserver(this); Are we removing the observer anywhere? Is there any way ComponentsUI is destroyed before the ComponentUpdateService? The observers are not refcounted, unfortunately. https://codereview.chromium.org/209313002/diff/160001/chrome/browser/ui/webui... chrome/browser/ui/webui/components_ui.cc:232: DCHECK(converted_event != NULL); It seems converted_event is some kind of a an event name. Perhaps call it event_name and define the constants somewhere? I appears these string literals have to be stable, a comment would be in order.
given the friendship situation it is ok if we expose CrxUpdateItem
Sorin, Ptal before I do nits, to see if it is in-line with what we discussed.
Thank you Shrikant. It goes in the right direction. https://codereview.chromium.org/209313002/diff/180001/chrome/browser/componen... File chrome/browser/component_updater/component_updater_service.cc (right): https://codereview.chromium.org/209313002/diff/180001/chrome/browser/componen... chrome/browser/component_updater/component_updater_service.cc:552: component_ids->push_back(GetCrxComponentID(item->component)); there is already an CrxUpdateItem::id member, we could use that instead . https://codereview.chromium.org/209313002/diff/180001/chrome/browser/componen... File chrome/browser/component_updater/component_updater_service.h (right): https://codereview.chromium.org/209313002/diff/180001/chrome/browser/componen... chrome/browser/component_updater/component_updater_service.h:205: virtual void GetComponentIDs(std::vector<std::string>* component_ids) = 0; Of course these two members would have to be declared in the private section of the the class since only the friends of the class could call them. https://codereview.chromium.org/209313002/diff/180001/chrome/browser/componen... chrome/browser/component_updater/component_updater_service.h:205: virtual void GetComponentIDs(std::vector<std::string>* component_ids) = 0; I would do this: virtual std::vector<std::string> GetComponentIDs() const = 0; virtual const CrxUpdateItem* GetComponentDetails( const std::string& component_id) const = 0;
ptal. https://codereview.chromium.org/209313002/diff/180001/chrome/browser/componen... File chrome/browser/component_updater/component_updater_service.cc (right): https://codereview.chromium.org/209313002/diff/180001/chrome/browser/componen... chrome/browser/component_updater/component_updater_service.cc:552: component_ids->push_back(GetCrxComponentID(item->component)); On 2014/05/03 01:09:06, Sorin Jianu wrote: > there is already an CrxUpdateItem::id member, we could use that instead . Done. https://codereview.chromium.org/209313002/diff/180001/chrome/browser/componen... File chrome/browser/component_updater/component_updater_service.h (right): https://codereview.chromium.org/209313002/diff/180001/chrome/browser/componen... chrome/browser/component_updater/component_updater_service.h:205: virtual void GetComponentIDs(std::vector<std::string>* component_ids) = 0; On 2014/05/03 01:09:06, Sorin Jianu wrote: > Of course these two members would have to be declared in the private section of > the the class since only the friends of the class could call them. Done. https://codereview.chromium.org/209313002/diff/180001/chrome/browser/componen... chrome/browser/component_updater/component_updater_service.h:205: virtual void GetComponentIDs(std::vector<std::string>* component_ids) = 0; On 2014/05/03 01:09:06, Sorin Jianu wrote: > I would do this: > > virtual std::vector<std::string> GetComponentIDs() const = 0; > > virtual const CrxUpdateItem* GetComponentDetails( > const std::string& component_id) const = 0; Done.
btw, I can remove timer thing if we can always make sure that after calling OnDemandUpdate observer will be updated with some status through callback as I am disabling button till then. On 2014/05/05 23:05:17, Shrikant Kelkar wrote: > ptal. > > https://codereview.chromium.org/209313002/diff/180001/chrome/browser/componen... > File chrome/browser/component_updater/component_updater_service.cc (right): > > https://codereview.chromium.org/209313002/diff/180001/chrome/browser/componen... > chrome/browser/component_updater/component_updater_service.cc:552: > component_ids->push_back(GetCrxComponentID(item->component)); > On 2014/05/03 01:09:06, Sorin Jianu wrote: > > there is already an CrxUpdateItem::id member, we could use that instead . > > Done. > > https://codereview.chromium.org/209313002/diff/180001/chrome/browser/componen... > File chrome/browser/component_updater/component_updater_service.h (right): > > https://codereview.chromium.org/209313002/diff/180001/chrome/browser/componen... > chrome/browser/component_updater/component_updater_service.h:205: virtual void > GetComponentIDs(std::vector<std::string>* component_ids) = 0; > On 2014/05/03 01:09:06, Sorin Jianu wrote: > > Of course these two members would have to be declared in the private section > of > > the the class since only the friends of the class could call them. > > Done. > > https://codereview.chromium.org/209313002/diff/180001/chrome/browser/componen... > chrome/browser/component_updater/component_updater_service.h:205: virtual void > GetComponentIDs(std::vector<std::string>* component_ids) = 0; > On 2014/05/03 01:09:06, Sorin Jianu wrote: > > I would do this: > > > > virtual std::vector<std::string> GetComponentIDs() const = 0; > > > > virtual const CrxUpdateItem* GetComponentDetails( > > const std::string& component_id) const = 0; > > Done.
Thank you Shrikant! https://codereview.chromium.org/209313002/diff/220001/chrome/browser/componen... File chrome/browser/component_updater/component_updater_service.cc (right): https://codereview.chromium.org/209313002/diff/220001/chrome/browser/componen... chrome/browser/component_updater/component_updater_service.cc:560: CrxUpdateItem* item = FindUpdateItemById(component_id); we could just return FindUpdateItemById(component_id); https://codereview.chromium.org/209313002/diff/220001/chrome/browser/componen... chrome/browser/component_updater/component_updater_service.cc:564: One extra empty line could be removed. https://codereview.chromium.org/209313002/diff/220001/chrome/browser/resource... File chrome/browser/resources/components.js (right): https://codereview.chromium.org/209313002/diff/220001/chrome/browser/resource... chrome/browser/resources/components.js:80: * component updater service doesn't fire onComponentEvent in response I've been thinking about this case. Indeed, we don't guarantee that a notification is fired every time checkUpdate is called. First, do we have to disable the button? The code in the component updater handles successive calls correctly. Related to this, there is currently a period of cooldown for calling on demand, which will interfere with the operation of this page. It only allows one call every 30 mins for each component. I am open to refactor the code so that the page has unrestricted access for OnDemand. Please double check this with Carlos and I will make the change. Second, we could add more notifications for state transitions in CrxUpdateService::ChangeItemState so that we also get events when the component goes to kChecking, kDownloading, kDownloadingDiff. Nice to have, I will add these events as well. In summary, we should try to not disable the button but make the underlying code work. https://codereview.chromium.org/209313002/diff/220001/chrome/browser/ui/webui... File chrome/browser/ui/webui/components_ui.cc (right): https://codereview.chromium.org/209313002/diff/220001/chrome/browser/ui/webui... chrome/browser/ui/webui/components_ui.cc:101: base::ListValue* list = ComponentsUI::LoadComponents(); Will the memory for |list| be correctly deallocated in this case. https://codereview.chromium.org/209313002/diff/220001/chrome/browser/ui/webui... chrome/browser/ui/webui/components_ui.cc:167: base::ListValue* component_list = new base::ListValue(); Who owns this memory? https://codereview.chromium.org/209313002/diff/220001/chrome/browser/ui/webui... chrome/browser/ui/webui/components_ui.cc:169: component_updater::CrxUpdateItem* item = cus->GetComponentDetails( This |item| could be const. Also, the result of calling cus->GetComponentDetails can be NULL if the component is not found (most likely because of a bug at this moment). One idea (it depends on personal taste) is to code this sitution defensively and not crash. https://codereview.chromium.org/209313002/diff/220001/chrome/browser/ui/webui... chrome/browser/ui/webui/components_ui.cc:172: base::DictionaryValue* component_entry = new base::DictionaryValue(); Who owns this memory allocation? https://codereview.chromium.org/209313002/diff/220001/chrome/browser/ui/webui... chrome/browser/ui/webui/components_ui.cc:194: std::string* converted_event) { Consider renaming the parameter to |event_string| or something similar.
Shrikant, one more thing: could you please "run git cl format" and "git cl lint"? We'd like to have a clean lint. We are striving for a clean clang-format but use discretion if the format changes break the style of surrounding code. Thank you!
ptal https://codereview.chromium.org/209313002/diff/220001/chrome/browser/componen... File chrome/browser/component_updater/component_updater_service.cc (right): https://codereview.chromium.org/209313002/diff/220001/chrome/browser/componen... chrome/browser/component_updater/component_updater_service.cc:560: CrxUpdateItem* item = FindUpdateItemById(component_id); On 2014/05/08 22:07:24, Sorin Jianu wrote: > we could just return FindUpdateItemById(component_id); Done. https://codereview.chromium.org/209313002/diff/220001/chrome/browser/componen... chrome/browser/component_updater/component_updater_service.cc:564: On 2014/05/08 22:07:24, Sorin Jianu wrote: > One extra empty line could be removed. Done. https://codereview.chromium.org/209313002/diff/220001/chrome/browser/resource... File chrome/browser/resources/components.js (right): https://codereview.chromium.org/209313002/diff/220001/chrome/browser/resource... chrome/browser/resources/components.js:80: * component updater service doesn't fire onComponentEvent in response On 2014/05/08 22:07:24, Sorin Jianu wrote: > I've been thinking about this case. Indeed, we don't guarantee that a > notification is fired every time checkUpdate is called. > > First, do we have to disable the button? The code in the component updater > handles successive calls correctly. Related to this, there is currently a period > of cooldown for calling on demand, which will interfere with the operation of > this page. It only allows one call every 30 mins for each component. I am open > to refactor the code so that the page has unrestricted access for OnDemand. > Please double check this with Carlos and I will make the change. > > Second, we could add more notifications for state transitions in > CrxUpdateService::ChangeItemState so that we also get events when the component > goes to kChecking, kDownloading, kDownloadingDiff. Nice to have, I will add > these events as well. > > In summary, we should try to not disable the button but make the underlying code > work. Done. https://codereview.chromium.org/209313002/diff/220001/chrome/browser/ui/webui... File chrome/browser/ui/webui/components_ui.cc (right): https://codereview.chromium.org/209313002/diff/220001/chrome/browser/ui/webui... chrome/browser/ui/webui/components_ui.cc:101: base::ListValue* list = ComponentsUI::LoadComponents(); On 2014/05/08 22:07:24, Sorin Jianu wrote: > Will the memory for |list| be correctly deallocated in this case. Originally I also was concerned about this, but it appears that once you attach this thing to DictionaryValue object, it deletes all that's attached and their contents during time of destruction. https://codereview.chromium.org/209313002/diff/220001/chrome/browser/ui/webui... chrome/browser/ui/webui/components_ui.cc:167: base::ListValue* component_list = new base::ListValue(); On 2014/05/08 22:07:24, Sorin Jianu wrote: > Who owns this memory? This instance is returned to caller, who attaches it to DictionaryValue. For how DictionaryValue destroys contents please please check it @ https://code.google.com/p/chromium/codesearch#chromium/src/base/values.cc&rcl... https://codereview.chromium.org/209313002/diff/220001/chrome/browser/ui/webui... chrome/browser/ui/webui/components_ui.cc:169: component_updater::CrxUpdateItem* item = cus->GetComponentDetails( On 2014/05/08 22:07:24, Sorin Jianu wrote: > This |item| could be const. Also, the result of calling cus->GetComponentDetails > can be NULL if the component is not found (most likely because of a bug at this > moment). One idea (it depends on personal taste) is to code this sitution > defensively and not crash. Done. https://codereview.chromium.org/209313002/diff/220001/chrome/browser/ui/webui... chrome/browser/ui/webui/components_ui.cc:172: base::DictionaryValue* component_entry = new base::DictionaryValue(); On 2014/05/08 22:07:24, Sorin Jianu wrote: > Who owns this memory allocation? component_list is owner here. https://codereview.chromium.org/209313002/diff/220001/chrome/browser/ui/webui... chrome/browser/ui/webui/components_ui.cc:194: std::string* converted_event) { On 2014/05/08 22:07:24, Sorin Jianu wrote: > Consider renaming the parameter to |event_string| or something similar. Done.
lgtm Thank you! https://codereview.chromium.org/209313002/diff/260001/chrome/browser/componen... File chrome/browser/component_updater/component_updater_service.h (right): https://codereview.chromium.org/209313002/diff/260001/chrome/browser/componen... chrome/browser/component_updater/component_updater_service.h:215: // Returns details about registered component. We need to add a comment here saying that the object returned by the function is owned by this class. I am not very fond of this particular function but I don't have a better idea. https://codereview.chromium.org/209313002/diff/260001/chrome/browser/ui/webui... File chrome/browser/ui/webui/components_ui.h (right): https://codereview.chromium.org/209313002/diff/260001/chrome/browser/ui/webui... chrome/browser/ui/webui/components_ui.h:38: static void ComponentEventToString(Events event, We could return a std::string just like the similar function @40 does. https://codereview.chromium.org/209313002/diff/260001/chrome/browser/ui/webui... chrome/browser/ui/webui/components_ui.h:39: std::string* converted_event); We need to rename |converted_event| here as well to match the name in the function definition.
Modified as per your comments. ptal. Thanks, +jhawkins Can you ptal? https://codereview.chromium.org/209313002/diff/260001/chrome/browser/componen... File chrome/browser/component_updater/component_updater_service.h (right): https://codereview.chromium.org/209313002/diff/260001/chrome/browser/componen... chrome/browser/component_updater/component_updater_service.h:215: // Returns details about registered component. On 2014/05/16 16:36:09, Sorin Jianu wrote: > We need to add a comment here saying that the object returned by the function is > owned by this class. I am not very fond of this particular function but I don't > have a better idea. Done. https://codereview.chromium.org/209313002/diff/260001/chrome/browser/ui/webui... File chrome/browser/ui/webui/components_ui.h (right): https://codereview.chromium.org/209313002/diff/260001/chrome/browser/ui/webui... chrome/browser/ui/webui/components_ui.h:38: static void ComponentEventToString(Events event, On 2014/05/16 16:36:09, Sorin Jianu wrote: > We could return a std::string just like the similar function @40 does. Done. https://codereview.chromium.org/209313002/diff/260001/chrome/browser/ui/webui... chrome/browser/ui/webui/components_ui.h:39: std::string* converted_event); On 2014/05/16 16:36:09, Sorin Jianu wrote: > We need to rename |converted_event| here as well to match the name in the > function definition. Modified to return string.
Which files are you asking me to review?
On 2014/05/19 02:56:31, James Hawkins wrote: > Which files are you asking me to review? Following files for which you are owner. chrome/browser/resources/components.html chrome/browser/resources/components.js chrome/browser/ui/webui/components_ui.h chrome/browser/ui/webui/components_ui.cc
Please send a screenshot of the new UI. https://codereview.chromium.org/209313002/diff/280001/chrome/browser/resource... File chrome/browser/resources/components.html (right): https://codereview.chromium.org/209313002/diff/280001/chrome/browser/resource... chrome/browser/resources/components.html:38: <div class="component-name no-components" jsdisplay="components.length === 0"> nit: 80 cols. https://codereview.chromium.org/209313002/diff/280001/chrome/browser/resource... chrome/browser/resources/components.html:67: <span>Status </span> Needs to be i18n-content. https://codereview.chromium.org/209313002/diff/280001/chrome/browser/resource... chrome/browser/resources/components.html:80: CHECK_UPDATE nit: Remove placeholder text. https://codereview.chromium.org/209313002/diff/280001/chrome/browser/resource... File chrome/browser/resources/components.js (right): https://codereview.chromium.org/209313002/diff/280001/chrome/browser/resource... chrome/browser/resources/components.js:17: var output = $('component-template').cloneNode(true); Why do you need to clone then remove the component-template element? https://codereview.chromium.org/209313002/diff/280001/chrome/browser/resource... chrome/browser/resources/components.js:97: $('status-' + String(node.id)).textContent = 'Checking for update...'; String needs to be translated, i.e., loadTimeData.getString. https://codereview.chromium.org/209313002/diff/280001/chrome/browser/ui/webui... File chrome/browser/ui/webui/components_ui.h (right): https://codereview.chromium.org/209313002/diff/280001/chrome/browser/ui/webui... chrome/browser/ui/webui/components_ui.h:34: // Override from ServiceObserver // ServiceObserver implementation. Per webui/ precedence.
ptal. https://codereview.chromium.org/209313002/diff/280001/chrome/browser/resource... File chrome/browser/resources/components.html (right): https://codereview.chromium.org/209313002/diff/280001/chrome/browser/resource... chrome/browser/resources/components.html:38: <div class="component-name no-components" jsdisplay="components.length === 0"> On 2014/05/21 16:21:37, James Hawkins wrote: > nit: 80 cols. Done. https://codereview.chromium.org/209313002/diff/280001/chrome/browser/resource... chrome/browser/resources/components.html:67: <span>Status </span> On 2014/05/21 16:21:37, James Hawkins wrote: > Needs to be i18n-content. Done. https://codereview.chromium.org/209313002/diff/280001/chrome/browser/resource... chrome/browser/resources/components.html:80: CHECK_UPDATE On 2014/05/21 16:21:37, James Hawkins wrote: > nit: Remove placeholder text. Done. https://codereview.chromium.org/209313002/diff/280001/chrome/browser/resource... File chrome/browser/resources/components.js (right): https://codereview.chromium.org/209313002/diff/280001/chrome/browser/resource... chrome/browser/resources/components.js:17: var output = $('component-template').cloneNode(true); On 2014/05/21 16:21:37, James Hawkins wrote: > Why do you need to clone then remove the component-template element? Not removing component-template, just removing 'hidden' attribute of cloned node. https://codereview.chromium.org/209313002/diff/280001/chrome/browser/resource... chrome/browser/resources/components.js:97: $('status-' + String(node.id)).textContent = 'Checking for update...'; On 2014/05/21 16:21:37, James Hawkins wrote: > String needs to be translated, i.e., loadTimeData.getString. Done. https://codereview.chromium.org/209313002/diff/280001/chrome/browser/ui/webui... File chrome/browser/ui/webui/components_ui.h (right): https://codereview.chromium.org/209313002/diff/280001/chrome/browser/ui/webui... chrome/browser/ui/webui/components_ui.h:34: // Override from ServiceObserver On 2014/05/21 16:21:37, James Hawkins wrote: > // ServiceObserver implementation. > > Per webui/ precedence. Done.
On 2014/05/23 03:49:23, Shrikant Kelkar wrote: > ptal. > > https://codereview.chromium.org/209313002/diff/280001/chrome/browser/resource... > File chrome/browser/resources/components.html (right): > > https://codereview.chromium.org/209313002/diff/280001/chrome/browser/resource... > chrome/browser/resources/components.html:38: <div class="component-name > no-components" jsdisplay="components.length === 0"> > On 2014/05/21 16:21:37, James Hawkins wrote: > > nit: 80 cols. > > Done. > > https://codereview.chromium.org/209313002/diff/280001/chrome/browser/resource... > chrome/browser/resources/components.html:67: <span>Status </span> > On 2014/05/21 16:21:37, James Hawkins wrote: > > Needs to be i18n-content. > > Done. > > https://codereview.chromium.org/209313002/diff/280001/chrome/browser/resource... > chrome/browser/resources/components.html:80: CHECK_UPDATE > On 2014/05/21 16:21:37, James Hawkins wrote: > > nit: Remove placeholder text. > > Done. > > https://codereview.chromium.org/209313002/diff/280001/chrome/browser/resource... > File chrome/browser/resources/components.js (right): > > https://codereview.chromium.org/209313002/diff/280001/chrome/browser/resource... > chrome/browser/resources/components.js:17: var output = > $('component-template').cloneNode(true); > On 2014/05/21 16:21:37, James Hawkins wrote: > > Why do you need to clone then remove the component-template element? > > Not removing component-template, just removing 'hidden' attribute of cloned > node. > > https://codereview.chromium.org/209313002/diff/280001/chrome/browser/resource... > chrome/browser/resources/components.js:97: $('status-' + > String(node.id)).textContent = 'Checking for update...'; > On 2014/05/21 16:21:37, James Hawkins wrote: > > String needs to be translated, i.e., loadTimeData.getString. > > Done. > > https://codereview.chromium.org/209313002/diff/280001/chrome/browser/ui/webui... > File chrome/browser/ui/webui/components_ui.h (right): > > https://codereview.chromium.org/209313002/diff/280001/chrome/browser/ui/webui... > chrome/browser/ui/webui/components_ui.h:34: // Override from ServiceObserver > On 2014/05/21 16:21:37, James Hawkins wrote: > > // ServiceObserver implementation. > > > > Per webui/ precedence. > > Done. For latest screenshot please refer to bug crbug.com/272540
https://codereview.chromium.org/209313002/diff/320001/chrome/browser/resource... File chrome/browser/resources/components.html (right): https://codereview.chromium.org/209313002/diff/320001/chrome/browser/resource... chrome/browser/resources/components.html:17: <div id="component-template" hidden> Please pull this out into a JS class. There are tons of examples in chrome/browser/resources, but a quick search for me pulled up [1]. [1] https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/res... https://codereview.chromium.org/209313002/diff/320001/chrome/browser/resource... chrome/browser/resources/components.html:49: <table> This isn't tabular data, so you should not being use <table>. https://codereview.chromium.org/209313002/diff/320001/chrome/browser/resource... chrome/browser/resources/components.html:71: jscontent="status" jsvalues=".id: 'status-' + id"> It appears this value is not translated. It is coming from ComponentsUI::ServiceStatusToString. https://codereview.chromium.org/209313002/diff/320001/chrome/browser/resource... File chrome/browser/resources/components.js (right): https://codereview.chromium.org/209313002/diff/320001/chrome/browser/resource... chrome/browser/resources/components.js:87: $('status-' + id).textContent = eventArgs['event']; I followed this trail, and it appears this text is not translated. It is coming from ComponentsUI::ComponentEventToString.
ptal. https://codereview.chromium.org/209313002/diff/320001/chrome/browser/resource... File chrome/browser/resources/components.html (right): https://codereview.chromium.org/209313002/diff/320001/chrome/browser/resource... chrome/browser/resources/components.html:17: <div id="component-template" hidden> On 2014/05/23 17:38:54, James Hawkins wrote: > Please pull this out into a JS class. > > There are tons of examples in chrome/browser/resources, but a quick search for > me pulled up [1]. > > [1] > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/res... Wondering what are the pros/cons of moving this to JS class? https://codereview.chromium.org/209313002/diff/320001/chrome/browser/resource... chrome/browser/resources/components.html:49: <table> On 2014/05/23 17:38:54, James Hawkins wrote: > This isn't tabular data, so you should not being use <table>. Done. https://codereview.chromium.org/209313002/diff/320001/chrome/browser/resource... chrome/browser/resources/components.html:71: jscontent="status" jsvalues=".id: 'status-' + id"> On 2014/05/23 17:38:54, James Hawkins wrote: > It appears this value is not translated. It is coming from > ComponentsUI::ServiceStatusToString. Done. https://codereview.chromium.org/209313002/diff/320001/chrome/browser/resource... File chrome/browser/resources/components.js (right): https://codereview.chromium.org/209313002/diff/320001/chrome/browser/resource... chrome/browser/resources/components.js:87: $('status-' + id).textContent = eventArgs['event']; On 2014/05/23 17:38:54, James Hawkins wrote: > I followed this trail, and it appears this text is not translated. It is coming > from ComponentsUI::ComponentEventToString. Done.
https://codereview.chromium.org/209313002/diff/340001/chrome/app/generated_re... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/209313002/diff/340001/chrome/app/generated_re... chrome/app/generated_resources.grd:5263: UpdaterStarted Spaces between the words, right? And probably sentence case, since this is displayed to the user. https://codereview.chromium.org/209313002/diff/340001/chrome/browser/resource... File chrome/browser/resources/components.css (right): https://codereview.chromium.org/209313002/diff/340001/chrome/browser/resource... chrome/browser/resources/components.css:189: display: block; Why is this necessary? https://codereview.chromium.org/209313002/diff/340001/chrome/browser/resource... File chrome/browser/resources/components.html (right): https://codereview.chromium.org/209313002/diff/340001/chrome/browser/resource... chrome/browser/resources/components.html:24: >TITLE</span> nit: Remove placeholder text. https://codereview.chromium.org/209313002/diff/340001/chrome/browser/resource... chrome/browser/resources/components.html:58: <span dir="ltr" How does this work in RTL? https://codereview.chromium.org/209313002/diff/340001/chrome/browser/resource... File chrome/browser/resources/components.js (right): https://codereview.chromium.org/209313002/diff/340001/chrome/browser/resource... chrome/browser/resources/components.js:79: * This event function is called from component ui indicating changed state Optional nit: s/ui/UI/ https://codereview.chromium.org/209313002/diff/340001/chrome/browser/resource... chrome/browser/resources/components.js:81: * @param {Object} eventArgs Contains event and component id. Component id is Optiona nit: s/id/ID/ https://codereview.chromium.org/209313002/diff/340001/chrome/browser/ui/webui... File chrome/browser/ui/webui/components_ui.cc (right): https://codereview.chromium.org/209313002/diff/340001/chrome/browser/ui/webui... chrome/browser/ui/webui/components_ui.cc:199: case COMPONENT_UPDATER_STARTED: Optional: Storing these in maps may make these methods simpler.
https://codereview.chromium.org/209313002/diff/320001/chrome/browser/resource... File chrome/browser/resources/components.html (right): https://codereview.chromium.org/209313002/diff/320001/chrome/browser/resource... chrome/browser/resources/components.html:17: <div id="component-template" hidden> On 2014/05/28 19:29:22, Shrikant Kelkar wrote: > On 2014/05/23 17:38:54, James Hawkins wrote: > > Please pull this out into a JS class. > > > > There are tons of examples in chrome/browser/resources, but a quick search for > > me pulled up [1]. > > > > [1] > > > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/res... > > Wondering what are the pros/cons of moving this to JS class? Since this is actually just structure (for the most part) without accompanying logic, I'm OK with leaving it as-is. Though in the future this type of stuff should be moved into a Web Component.
ptal. https://codereview.chromium.org/209313002/diff/340001/chrome/app/generated_re... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/209313002/diff/340001/chrome/app/generated_re... chrome/app/generated_resources.grd:5263: UpdaterStarted On 2014/05/28 23:01:11, James Hawkins wrote: > Spaces between the words, right? And probably sentence case, since this is > displayed to the user. Done. https://codereview.chromium.org/209313002/diff/340001/chrome/browser/resource... File chrome/browser/resources/components.css (right): https://codereview.chromium.org/209313002/diff/340001/chrome/browser/resource... chrome/browser/resources/components.css:189: display: block; On 2014/05/28 23:01:11, James Hawkins wrote: > Why is this necessary? Removed https://codereview.chromium.org/209313002/diff/340001/chrome/browser/resource... File chrome/browser/resources/components.html (right): https://codereview.chromium.org/209313002/diff/340001/chrome/browser/resource... chrome/browser/resources/components.html:24: >TITLE</span> On 2014/05/28 23:01:11, James Hawkins wrote: > nit: Remove placeholder text. Done. https://codereview.chromium.org/209313002/diff/340001/chrome/browser/resource... chrome/browser/resources/components.html:58: <span dir="ltr" On 2014/05/28 23:01:11, James Hawkins wrote: > How does this work in RTL? Not sure about RTL case. Will investigate and fix in future. https://codereview.chromium.org/209313002/diff/340001/chrome/browser/resource... File chrome/browser/resources/components.js (right): https://codereview.chromium.org/209313002/diff/340001/chrome/browser/resource... chrome/browser/resources/components.js:79: * This event function is called from component ui indicating changed state On 2014/05/28 23:01:11, James Hawkins wrote: > Optional nit: s/ui/UI/ Done. https://codereview.chromium.org/209313002/diff/340001/chrome/browser/resource... chrome/browser/resources/components.js:81: * @param {Object} eventArgs Contains event and component id. Component id is On 2014/05/28 23:01:11, James Hawkins wrote: > Optiona nit: s/id/ID/ Done. https://codereview.chromium.org/209313002/diff/340001/chrome/browser/ui/webui... File chrome/browser/ui/webui/components_ui.cc (right): https://codereview.chromium.org/209313002/diff/340001/chrome/browser/ui/webui... chrome/browser/ui/webui/components_ui.cc:199: case COMPONENT_UPDATER_STARTED: On 2014/05/28 23:01:11, James Hawkins wrote: > Optional: Storing these in maps may make these methods simpler. Will keep them as it is for now.
https://codereview.chromium.org/209313002/diff/340001/chrome/browser/resource... File chrome/browser/resources/components.html (right): https://codereview.chromium.org/209313002/diff/340001/chrome/browser/resource... chrome/browser/resources/components.html:58: <span dir="ltr" On 2014/05/28 23:14:45, Shrikant Kelkar wrote: > On 2014/05/28 23:01:11, James Hawkins wrote: > > How does this work in RTL? > > Not sure about RTL case. Will investigate and fix in future. RTL is first class. So we need to make sure it works before committing.
As you asked I talked with person who added dir=ltr, here is what he recalls from long time back CL, " I believe the reason for the LTR is that the actual plugin names will be LTR, so if we don't also make the header LTR, it'll be oppositely-aligned with the names in an RTL context" As I mentioned to you on Chat, I will remove this dir=ltr attribute from here (where localized contents are filled) and keep it for non localized content. On May 28, 2014 4:17 PM, <jhawkins@chromium.org> wrote: > > https://codereview.chromium.org/209313002/diff/340001/ > chrome/browser/resources/components.html > File chrome/browser/resources/components.html (right): > > https://codereview.chromium.org/209313002/diff/340001/ > chrome/browser/resources/components.html#newcode58 > chrome/browser/resources/components.html:58: <span dir="ltr" > On 2014/05/28 23:14:45, Shrikant Kelkar wrote: > >> On 2014/05/28 23:01:11, James Hawkins wrote: >> > How does this work in RTL? >> > > Not sure about RTL case. Will investigate and fix in future. >> > > RTL is first class. So we need to make sure it works before committing. > > https://codereview.chromium.org/209313002/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
https://codereview.chromium.org/209313002/diff/360001/chrome/browser/resource... File chrome/browser/resources/components.html (right): https://codereview.chromium.org/209313002/diff/360001/chrome/browser/resource... chrome/browser/resources/components.html:58: <span dir="ltr" Because the text in this span is localized (we pull translations from generated_resources), this needs to be RTL compatible, so we need to remove the dir="ltr".
ptal. https://codereview.chromium.org/209313002/diff/360001/chrome/browser/resource... File chrome/browser/resources/components.html (right): https://codereview.chromium.org/209313002/diff/360001/chrome/browser/resource... chrome/browser/resources/components.html:58: <span dir="ltr" On 2014/05/29 16:22:23, James Hawkins wrote: > Because the text in this span is localized (we pull translations from > generated_resources), this needs to be RTL compatible, so we need to remove the > dir="ltr". Done.
lgtm
The CQ bit was checked by shrikant@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/shrikant@chromium.org/209313002/380001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: android_clang_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_clang_dbg/bui...) android_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_dbg/builds/19...) linux_chromium_chromeos_clang_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...) 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_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...) mac_chromium_compile_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_compile_...) mac_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...) linux_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...) mac_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/10874)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_dbg/builds/19...)
The CQ bit was checked by shrikant@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/shrikant@chromium.org/209313002/420001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: android_dbg_triggered_tests on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...)
Message was sent while issue was closed.
Change committed as 274385 |
