Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
342003005:
Show alert failure for reloading unpacked extensions with bad manifest (Closed)
DescriptionShow alert failure for reloading unpacked extensions with bad manifest
files when refreshing extensions page.
Screenshot:
http://i.imgur.com/02tzoUK.png
BUG=375276
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=283852
Patch Set 1 #
Total comments: 1
Patch Set 2 : Cleanup #
Total comments: 21
Patch Set 3 : Minor changes, moved observer class #
Total comments: 3
Patch Set 4 : Synchronized page loading, support multiple load errors #
Total comments: 68
Patch Set 5 : Addressed patch set 4 comments #
Total comments: 51
Patch Set 6 : Minor changes #Patch Set 7 : Fixed FailureData struct #Patch Set 8 : Fixed be_noisy, support interaction with multiple failures #
Total comments: 29
Patch Set 9 : Moar minor changes #Patch Set 10 : Moved path handling back to C++ #
Total comments: 21
Patch Set 11 : Minor logic changes, comments, reverted ReloadExtension calls #Patch Set 12 : Added ReloadExtensionImpl, enhanced loader handler to send ListValue of failures #
Total comments: 38
Patch Set 13 : Minor changes, HTML list #
Total comments: 20
Patch Set 14 : Pointer scope, page loading notification, other minor changes #
Total comments: 21
Patch Set 15 : Comments, conditionals, nits, etc #
Total comments: 1
Patch Set 16 : Styling #Patch Set 17 : Prettified failure path #
Total comments: 29
Patch Set 18 : Addressed finnur's comments #
Total comments: 3
Patch Set 19 : Update comments and user facing text #
Total comments: 6
Patch Set 20 : Final comment nits #Messages
Total messages: 59 (0 generated)
https://codereview.chromium.org/342003005/diff/1/chrome/browser/ui/webui/exte... File chrome/browser/ui/webui/extensions/extension_loader_handler.cc (right): https://codereview.chromium.org/342003005/diff/1/chrome/browser/ui/webui/exte... chrome/browser/ui/webui/extensions/extension_loader_handler.cc:233: /* Replacing the PostTaskAndReplyWithResult with this commented section results in an error: "Function marked as IO-only was called from a thread that disallows IO! If this thread really should be allowed to make IO calls, adjust the call to base::ThreadRestrictions::SetIOAllowed() in this thread's startup."
@kalman, What do you think of this? https://codereview.chromium.org/342003005/diff/20001/chrome/browser/ui/webui/... File chrome/browser/ui/webui/extensions/extension_loader_handler.cc (right): https://codereview.chromium.org/342003005/diff/20001/chrome/browser/ui/webui/... chrome/browser/ui/webui/extensions/extension_loader_handler.cc:241: web_ui()->CallJavascriptFunction( There is an issue with synchronicity here. On a page reload, if an unpacked extension triggers an OnLoadError, this CallJavascriptFunction call will trigger a javascript function call before 'extensions' exists on the page (before the DOM is fully loaded), which results in an error: "Uncaught ReferenceError: extensions is not defined" If there is some way to delay this call until the page is fully loaded, this will work for a page refresh. As of right now, this code simply fixes the issue of old warnings when an unpacked extension is manually reloaded and triggers an OnLoadFailure. The new UI now shows up in this case.
Devlin will be reviewing this eventually so passing the question to him.
https://codereview.chromium.org/342003005/diff/20001/chrome/browser/extension... File chrome/browser/extensions/extension_error_reporter.cc (right): https://codereview.chromium.org/342003005/diff/20001/chrome/browser/extension... chrome/browser/extensions/extension_error_reporter.cc:6: #include "chrome/browser/extensions/extension_error_reporter_observer.h" Only the associated .h file goes at the top, all others in the main bunch. https://codereview.chromium.org/342003005/diff/20001/chrome/browser/extension... chrome/browser/extensions/extension_error_reporter.cc:59: TriggerOnLoadFailure(extension_path, error); You can just inline the notification here. https://codereview.chromium.org/342003005/diff/20001/chrome/browser/extension... chrome/browser/extensions/extension_error_reporter.cc:92: void ExtensionErrorReporter::AddObserver( Methods should be defined in the .cc in the same order they were declared in the .h. https://codereview.chromium.org/342003005/diff/20001/chrome/browser/extension... File chrome/browser/extensions/extension_error_reporter_observer.h (right): https://codereview.chromium.org/342003005/diff/20001/chrome/browser/extension... chrome/browser/extensions/extension_error_reporter_observer.h:12: class ExtensionErrorReporterObserver { This class is small enough and should have few enough implementors that it can be nested as an inner class in ExtensionErrorReporter. https://codereview.chromium.org/342003005/diff/20001/chrome/browser/extension... chrome/browser/extensions/extension_error_reporter_observer.h:17: const std::string& error) {} Since this is the only method, we should make it pure virtual. https://codereview.chromium.org/342003005/diff/20001/chrome/browser/extension... File chrome/browser/extensions/unpacked_installer.cc (right): https://codereview.chromium.org/342003005/diff/20001/chrome/browser/extension... chrome/browser/extensions/unpacked_installer.cc:117: be_noisy_on_failure_(false), Won't this potentially make other things which should be noisy, not? E.g. if we load an extension from the Chrome Apps & Extensions Developer Tool? https://codereview.chromium.org/342003005/diff/20001/chrome/browser/extension... chrome/browser/extensions/unpacked_installer.cc:299: if (!on_failure_callback_.is_null()) I could be wrong, but I think the only class that ever set this was the LoaderHandler. If so, you can remove it. https://codereview.chromium.org/342003005/diff/20001/chrome/browser/ui/webui/... File chrome/browser/ui/webui/extensions/extension_loader_handler.cc (right): https://codereview.chromium.org/342003005/diff/20001/chrome/browser/ui/webui/... chrome/browser/ui/webui/extensions/extension_loader_handler.cc:34: class ExtensionErrorReporter; Don't need to forward declare classes either forward declared in this .h or whose headers have been included. https://codereview.chromium.org/342003005/diff/20001/chrome/browser/ui/webui/... chrome/browser/ui/webui/extensions/extension_loader_handler.cc:241: web_ui()->CallJavascriptFunction( On 2014/06/21 01:22:07, gpdavis wrote: > There is an issue with synchronicity here. On a page reload, if an unpacked > extension triggers an OnLoadError, this CallJavascriptFunction call will trigger > a javascript function call before 'extensions' exists on the page (before the > DOM is fully loaded), which results in an error: > > "Uncaught ReferenceError: extensions is not defined" > > If there is some way to delay this call until the page is fully loaded, this > will work for a page refresh. As of right now, this code simply fixes the issue > of old warnings when an unpacked extension is manually reloaded and triggers an > OnLoadFailure. The new UI now shows up in this case. What I would envision would be keeping track of all extensions which failed to load, and showing the UI for it/them once the page fully loads. (We do also need to figure out what we're going to do for multiple load failures - is the current behavior that it pops up multiple dialogs?) https://codereview.chromium.org/342003005/diff/20001/chrome/browser/ui/webui/... File chrome/browser/ui/webui/extensions/extension_loader_handler.h (right): https://codereview.chromium.org/342003005/diff/20001/chrome/browser/ui/webui/... chrome/browser/ui/webui/extensions/extension_loader_handler.h:16: #include "chrome/browser/extensions/extension_error_reporter.h" FYI, you wouldn't need this include if the observer is in its own file (but you will once it moves to the main file). https://codereview.chromium.org/342003005/diff/20001/chrome/browser/ui/webui/... chrome/browser/ui/webui/extensions/extension_loader_handler.h:30: class ExtensionErrorReporterObserver; You don't have to forward declare classes whose .h's you include. https://codereview.chromium.org/342003005/diff/20001/chrome/browser/ui/webui/... chrome/browser/ui/webui/extensions/extension_loader_handler.h:49: virtual void OnLoadFailure(const base::FilePath& file_path, No reason to have this public. https://codereview.chromium.org/342003005/diff/20001/chrome/browser/ui/webui/... chrome/browser/ui/webui/extensions/extension_loader_handler.h:65: // void OnLoadFailure(const base::FilePath& file_path, const std::string& Remove dead code.
https://codereview.chromium.org/342003005/diff/20001/chrome/browser/extension... File chrome/browser/extensions/extension_error_reporter.cc (right): https://codereview.chromium.org/342003005/diff/20001/chrome/browser/extension... chrome/browser/extensions/extension_error_reporter.cc:92: void ExtensionErrorReporter::AddObserver( They are implemented in the same order as in the header, but should the AddObserver and RemoveObserver methods be higher up in the header and cc file? https://codereview.chromium.org/342003005/diff/20001/chrome/browser/extension... File chrome/browser/extensions/unpacked_installer.cc (right): https://codereview.chromium.org/342003005/diff/20001/chrome/browser/extension... chrome/browser/extensions/unpacked_installer.cc:117: be_noisy_on_failure_(false), Yes, that is true. But do we ever want it to be noisy, considering the goal here is to have the UI show up whenever there is a failure? Under what circumstances exactly do we want this to be true or false? https://codereview.chromium.org/342003005/diff/20001/chrome/browser/ui/webui/... File chrome/browser/ui/webui/extensions/extension_loader_handler.cc (right): https://codereview.chromium.org/342003005/diff/20001/chrome/browser/ui/webui/... chrome/browser/ui/webui/extensions/extension_loader_handler.cc:241: web_ui()->CallJavascriptFunction( The current behavior is that only the last one is shown, because the template for this only has the architecture to support displaying one error. This is definitely a big decision that needs to be made. I do like the idea of keeping track of all the extensions that failed to load. So maybe the best idea would be to do that, and then send a message once the page loads to display information about any extensions that failed to load while the page was rendering? Doesn't seem like an incredibly difficult thing to implement; I was just planning on getting it to work for one extension before we move on to determining what should be done if multiple extensions fail. Maybe we can show the UI we have now for one failure, but if multiple fail, we can display an additional section that indicates that more extensions failed to load but that we aren't showing exactly where in the manifest there's a problem. So then one problem can be displayed at a time.
https://codereview.chromium.org/342003005/diff/20001/chrome/browser/extension... File chrome/browser/extensions/extension_error_reporter.cc (right): https://codereview.chromium.org/342003005/diff/20001/chrome/browser/extension... chrome/browser/extensions/extension_error_reporter.cc:92: void ExtensionErrorReporter::AddObserver( On 2014/06/25 00:21:01, gpdavis wrote: > They are implemented in the same order as in the header, but should the > AddObserver and RemoveObserver methods be higher up in the header and cc file? Whoops, you're right, my bad. https://codereview.chromium.org/342003005/diff/20001/chrome/browser/extension... File chrome/browser/extensions/unpacked_installer.cc (right): https://codereview.chromium.org/342003005/diff/20001/chrome/browser/extension... chrome/browser/extensions/unpacked_installer.cc:117: be_noisy_on_failure_(false), On 2014/06/25 00:21:01, gpdavis wrote: > Yes, that is true. But do we ever want it to be noisy, considering the goal > here is to have the UI show up whenever there is a failure? Under what > circumstances exactly do we want this to be true or false? We want this to be noisy whenever: a) It is current noisy, and b) We won't show the new UI. A little searching around the code base will tell you exactly when that is, but the main one I can think of is the one I mentioned before - when we load an extension via Chrome Apps and Extensions Developer Tool. https://codereview.chromium.org/342003005/diff/20001/chrome/browser/ui/webui/... File chrome/browser/ui/webui/extensions/extension_loader_handler.cc (right): https://codereview.chromium.org/342003005/diff/20001/chrome/browser/ui/webui/... chrome/browser/ui/webui/extensions/extension_loader_handler.cc:241: web_ui()->CallJavascriptFunction( On 2014/06/25 00:21:01, gpdavis wrote: > The current behavior is that only the last one is shown, because the template > for this only has the architecture to support displaying one error. This is > definitely a big decision that needs to be made. > > I do like the idea of keeping track of all the extensions that failed to load. > So maybe the best idea would be to do that, and then send a message once the > page loads to display information about any extensions that failed to load while > the page was rendering? Doesn't seem like an incredibly difficult thing to > implement; I was just planning on getting it to work for one extension before we > move on to determining what should be done if multiple extensions fail. Maybe > we can show the UI we have now for one failure, but if multiple fail, we can > display an additional section that indicates that more extensions failed to load > but that we aren't showing exactly where in the manifest there's a problem. So > then one problem can be displayed at a time. Sorry, I was unclear. By "current behavior", I meant "behavior in released chrome", vs "behavior in this patch". That is, if we currently break three extensions, do we show a modal dialog for each? And yeah, keeping track of extensions and then sending a message once the page is ready sounds good. https://codereview.chromium.org/342003005/diff/40001/chrome/browser/extension... File chrome/browser/extensions/extension_error_reporter.h (right): https://codereview.chromium.org/342003005/diff/40001/chrome/browser/extension... chrome/browser/extensions/extension_error_reporter.h:32: class ExtensionErrorReporterObserver { Since any other classes will prefix this by ExtensionErrorReporter, we can just have the class name be "Observer". https://codereview.chromium.org/342003005/diff/40001/chrome/browser/extension... chrome/browser/extensions/extension_error_reporter.h:38: }; nit: newline after class declaration. https://codereview.chromium.org/342003005/diff/40001/chrome/browser/ui/webui/... File chrome/browser/ui/webui/extensions/extension_loader_handler.h (right): https://codereview.chromium.org/342003005/diff/40001/chrome/browser/ui/webui/... chrome/browser/ui/webui/extensions/extension_loader_handler.h:60: // void OnLoadFailure(const base::FilePath& file_path, const std::string& still have dead code here.
Hey Devlin, I implemented an idea I had for handling multiple load failures, and found a way to synchronize the load failure reports with the page loading. Can you take a look? Thanks
By the way, it's good practice to go through the comments left on the patch and say "done" or respond to a question or something so that reviewers can easily make sure you saw all comments, and you can make sure you don't forget anything. :) https://codereview.chromium.org/342003005/diff/20001/chrome/browser/ui/webui/... File chrome/browser/ui/webui/extensions/extension_loader_handler.cc (right): https://codereview.chromium.org/342003005/diff/20001/chrome/browser/ui/webui/... chrome/browser/ui/webui/extensions/extension_loader_handler.cc:241: web_ui()->CallJavascriptFunction( On 2014/06/25 19:49:36, Devlin wrote: > On 2014/06/25 00:21:01, gpdavis wrote: > > The current behavior is that only the last one is shown, because the template > > for this only has the architecture to support displaying one error. This is > > definitely a big decision that needs to be made. > > > > I do like the idea of keeping track of all the extensions that failed to load. > > > So maybe the best idea would be to do that, and then send a message once the > > page loads to display information about any extensions that failed to load > while > > the page was rendering? Doesn't seem like an incredibly difficult thing to > > implement; I was just planning on getting it to work for one extension before > we > > move on to determining what should be done if multiple extensions fail. Maybe > > we can show the UI we have now for one failure, but if multiple fail, we can > > display an additional section that indicates that more extensions failed to > load > > but that we aren't showing exactly where in the manifest there's a problem. > So > > then one problem can be displayed at a time. > > Sorry, I was unclear. By "current behavior", I meant "behavior in released > chrome", vs "behavior in this patch". That is, if we currently break three > extensions, do we show a modal dialog for each? > > And yeah, keeping track of extensions and then sending a message once the page > is ready sounds good. Still waiting on an answer for current behavior? https://codereview.chromium.org/342003005/diff/60001/chrome/browser/extension... File chrome/browser/extensions/extension_error_reporter.h (right): https://codereview.chromium.org/342003005/diff/60001/chrome/browser/extension... chrome/browser/extensions/extension_error_reporter.h:32: public: nit: one space indentation for public:, private:, protected:. https://codereview.chromium.org/342003005/diff/60001/chrome/browser/extension... chrome/browser/extensions/extension_error_reporter.h:35: virtual void OnLoadFailure(const base::FilePath& extension_path, nit: comment. https://codereview.chromium.org/342003005/diff/60001/chrome/browser/extension... File chrome/browser/extensions/extension_service.cc (right): https://codereview.chromium.org/342003005/diff/60001/chrome/browser/extension... chrome/browser/extensions/extension_service.cc:687: unpacked_installer->set_be_noisy_on_failure(false); Won't this still have problems with things other than chrome:extensions reloading extensions? https://codereview.chromium.org/342003005/diff/60001/chrome/browser/extension... File chrome/browser/extensions/unpacked_installer.cc (right): https://codereview.chromium.org/342003005/diff/60001/chrome/browser/extension... chrome/browser/extensions/unpacked_installer.cc:309: nit: remove extraneous newline. https://codereview.chromium.org/342003005/diff/60001/chrome/browser/resources... File chrome/browser/resources/extensions/extension_load_error.html (right): https://codereview.chromium.org/342003005/diff/60001/chrome/browser/resources... chrome/browser/resources/extensions/extension_load_error.html:21: <div id="extension-load-error-additional"> these don't both need ids. https://codereview.chromium.org/342003005/diff/60001/chrome/browser/resources... chrome/browser/resources/extensions/extension_load_error.html:23: </div> Any UI for this will have to go through review... but we'll get to that later. https://codereview.chromium.org/342003005/diff/60001/chrome/browser/resources... File chrome/browser/resources/extensions/extension_loader.js (right): https://codereview.chromium.org/342003005/diff/60001/chrome/browser/resources... chrome/browser/resources/extensions/extension_loader.js:40: this.additional_ = comments. https://codereview.chromium.org/342003005/diff/60001/chrome/browser/resources... chrome/browser/resources/extensions/extension_loader.js:81: showAdditional: function(failures) { comments. https://codereview.chromium.org/342003005/diff/60001/chrome/browser/resources... chrome/browser/resources/extensions/extension_loader.js:134: notifyAdditionalFailures: function(failures) { comments. https://codereview.chromium.org/342003005/diff/60001/chrome/browser/resources... chrome/browser/resources/extensions/extension_loader.js:150: ExtensionLoader.notifyAdditional = function(failures) { comments. https://codereview.chromium.org/342003005/diff/60001/chrome/browser/resources... File chrome/browser/resources/extensions/extensions.js (right): https://codereview.chromium.org/342003005/diff/60001/chrome/browser/resources... chrome/browser/resources/extensions/extensions.js:18: document.addEventListener('DOMContentLoaded', function() { ExtensionLoader should have as few dependencies on things like full extensions.js as possible. Why not notify from the ExtensionLoader js class? https://codereview.chromium.org/342003005/diff/60001/chrome/browser/resources... chrome/browser/resources/extensions/extensions.js:19: chrome.send('extensionLoaderDisplayFailures'); nit: indentation. https://codereview.chromium.org/342003005/diff/60001/chrome/browser/resources... chrome/browser/resources/extensions/extensions.js:22: window.addEventListener('beforeunload', function() { nit: indentation. https://codereview.chromium.org/342003005/diff/60001/chrome/browser/ui/webui/... File chrome/browser/ui/webui/extensions/extension_loader_handler.cc (right): https://codereview.chromium.org/342003005/diff/60001/chrome/browser/ui/webui/... chrome/browser/ui/webui/extensions/extension_loader_handler.cc:134: public: fix indentation. https://codereview.chromium.org/342003005/diff/60001/chrome/browser/ui/webui/... chrome/browser/ui/webui/extensions/extension_loader_handler.cc:135: explicit FailureData(base::FilePath path, std::string error, nit: one param per line. https://codereview.chromium.org/342003005/diff/60001/chrome/browser/ui/webui/... chrome/browser/ui/webui/extensions/extension_loader_handler.cc:135: explicit FailureData(base::FilePath path, std::string error, const & on non-simple arguments. https://codereview.chromium.org/342003005/diff/60001/chrome/browser/ui/webui/... chrome/browser/ui/webui/extensions/extension_loader_handler.cc:142: base::FilePath file_path; entirely public classes are what structs are for ;) https://codereview.chromium.org/342003005/diff/60001/chrome/browser/ui/webui/... chrome/browser/ui/webui/extensions/extension_loader_handler.cc:151: weak_ptr_factory_(this), weak_ptr_factory_ should be the final data member. https://codereview.chromium.org/342003005/diff/60001/chrome/browser/ui/webui/... chrome/browser/ui/webui/extensions/extension_loader_handler.cc:219: if (!failures_.empty()) { instead of: function { if (condition) { <lots of lines> } } prefer: function { if (!condition) return <lots of lines> } https://codereview.chromium.org/342003005/diff/60001/chrome/browser/ui/webui/... chrome/browser/ui/webui/extensions/extension_loader_handler.cc:220: FailureData *failure = failures_[0]; nit: FailureData* failure https://codereview.chromium.org/342003005/diff/60001/chrome/browser/ui/webui/... chrome/browser/ui/webui/extensions/extension_loader_handler.cc:221: NotifyFrontendOfFailure(failure->file_path, failure->error, one arg per line. https://codereview.chromium.org/342003005/diff/60001/chrome/browser/ui/webui/... chrome/browser/ui/webui/extensions/extension_loader_handler.cc:224: std::string list("Additional failures:"); This needs to be i18n. https://codereview.chromium.org/342003005/diff/60001/chrome/browser/ui/webui/... chrome/browser/ui/webui/extensions/extension_loader_handler.cc:225: for (size_t i = 1; i < failures_.size(); i++) { ++i. https://codereview.chromium.org/342003005/diff/60001/chrome/browser/ui/webui/... chrome/browser/ui/webui/extensions/extension_loader_handler.cc:228: list.append(failure->file_path.value()); Use one of the display functions in FilePath. https://codereview.chromium.org/342003005/diff/60001/chrome/browser/ui/webui/... chrome/browser/ui/webui/extensions/extension_loader_handler.cc:230: failures_.clear(); Gaaah! Leaaaaaaak! ;) But really. Leaks are bad. :) https://codereview.chromium.org/342003005/diff/60001/chrome/browser/ui/webui/... chrome/browser/ui/webui/extensions/extension_loader_handler.cc:234: list_value); If we go with this approach for multi errors, I think it would be better to send all errors in one big batch, and let the UI work it out. This has the advantage of, for instance, allowing us to display the next error if the user choose "I give up" on the first one. https://codereview.chromium.org/342003005/diff/60001/chrome/browser/ui/webui/... chrome/browser/ui/webui/extensions/extension_loader_handler.cc:247: display_ready_ = true; This seems like an odd time to set |display_ready_|... https://codereview.chromium.org/342003005/diff/60001/chrome/browser/ui/webui/... chrome/browser/ui/webui/extensions/extension_loader_handler.cc:308: display_ready_ = false; This too seems like an odd time to set display ready. I assume these are just so that you know when to notify of additional failures vs the primary failure? There should be a better way of doing this. https://codereview.chromium.org/342003005/diff/60001/chrome/browser/ui/webui/... File chrome/browser/ui/webui/extensions/extension_loader_handler.h (right): https://codereview.chromium.org/342003005/diff/60001/chrome/browser/ui/webui/... chrome/browser/ui/webui/extensions/extension_loader_handler.h:52: std::vector<FailureData*> failures_; member variables at the bottom. https://codereview.chromium.org/342003005/diff/60001/chrome/browser/ui/webui/... chrome/browser/ui/webui/extensions/extension_loader_handler.h:71: // Called when an unpacked extension fails to load. This comment should be updated to be: "ExtensionErrorReporter::Observer implementation."
Thanks for the review! I'll make sure to address all the comments. Here's some of my thoughts about the things you pointed out. https://codereview.chromium.org/342003005/diff/60001/chrome/browser/extension... File chrome/browser/extensions/extension_service.cc (right): https://codereview.chromium.org/342003005/diff/60001/chrome/browser/extension... chrome/browser/extensions/extension_service.cc:687: unpacked_installer->set_be_noisy_on_failure(false); Oops, you're absolutely right. There must be some way to make this determination. The problem is that this UnpackedInstaller gets created and used right here, so this is the only opportunity to tell it not to be noisy. Perhaps we could add an extra boolean argument to ReloadExtension? That doesn't seem entirely unreasonable of a thing to add to a ReloadExtension call. What do you think? https://codereview.chromium.org/342003005/diff/60001/chrome/browser/resources... File chrome/browser/resources/extensions/extension_load_error.html (right): https://codereview.chromium.org/342003005/diff/60001/chrome/browser/resources... chrome/browser/resources/extensions/extension_load_error.html:23: </div> Understood. I figured if I could get the necessary architectural changes done, the UI stuff could be figured out afterward. https://codereview.chromium.org/342003005/diff/60001/chrome/browser/resources... File chrome/browser/resources/extensions/extensions.js (right): https://codereview.chromium.org/342003005/diff/60001/chrome/browser/resources... chrome/browser/resources/extensions/extensions.js:18: document.addEventListener('DOMContentLoaded', function() { I'll give that a try and see if it works the same. My idea for this was to send the proper signals that tell the error reporter whether the UI is ready to receive these error messages. https://codereview.chromium.org/342003005/diff/60001/chrome/browser/ui/webui/... File chrome/browser/ui/webui/extensions/extension_loader_handler.cc (right): https://codereview.chromium.org/342003005/diff/60001/chrome/browser/ui/webui/... chrome/browser/ui/webui/extensions/extension_loader_handler.cc:142: base::FilePath file_path; That was what I started with, but I wasn't sure if I could have a vector of structs, so I made it a class instead. I suppose there should be no reason this wouldn't work with structs. I'll have to play around with it and see if I can get that switched over. https://codereview.chromium.org/342003005/diff/60001/chrome/browser/ui/webui/... chrome/browser/ui/webui/extensions/extension_loader_handler.cc:219: if (!failures_.empty()) { That is indeed much better. Will fix (y) https://codereview.chromium.org/342003005/diff/60001/chrome/browser/ui/webui/... chrome/browser/ui/webui/extensions/extension_loader_handler.cc:228: list.append(failure->file_path.value()); Sounds good. LossyDisplayName seem reasonable? https://codereview.chromium.org/342003005/diff/60001/chrome/browser/ui/webui/... chrome/browser/ui/webui/extensions/extension_loader_handler.cc:230: failures_.clear(); I had a feeling there was going to be a memory leak here. What would be the preferred way to free up the memory of those references before clearing the vector? https://codereview.chromium.org/342003005/diff/60001/chrome/browser/ui/webui/... chrome/browser/ui/webui/extensions/extension_loader_handler.cc:234: list_value); This is certainly going to require heavier UI adjustments. Knowing that UI modifications need to be specifically reviewed, I would like to have somebody work with me to figure out the most reasonable solution. But we can cross that bridge when we come to it. It would be a relatively minor adjustment to make this send off all at once instead of the way it currently is. How about we make that determination once the primary architectural changes look good? https://codereview.chromium.org/342003005/diff/60001/chrome/browser/ui/webui/... chrome/browser/ui/webui/extensions/extension_loader_handler.cc:308: display_ready_ = false; This was initially done because I started off implementing this so that we never assume the display is ready unless the flag was explicitly set before the call, so I reset it beforehand. This is also the reason why it's set to true in LoadUnpackedExtensionImple; if the user clicked the button to load the extension, it follows that the UI must be ready. But since I added the events to the JS that notified this class when the UI was ready or not ready, this is no longer necessary. I'll look into pulling these out safely.
https://codereview.chromium.org/342003005/diff/60001/chrome/browser/extension... File chrome/browser/extensions/extension_error_reporter.h (right): https://codereview.chromium.org/342003005/diff/60001/chrome/browser/extension... chrome/browser/extensions/extension_error_reporter.h:32: public: On 2014/06/27 22:31:08, Devlin wrote: > nit: one space indentation for public:, private:, protected:. Cursed auto indentation ;) Done. https://codereview.chromium.org/342003005/diff/60001/chrome/browser/extension... chrome/browser/extensions/extension_error_reporter.h:35: virtual void OnLoadFailure(const base::FilePath& extension_path, // Observer function called when an unpacked extension fails to load. Seem reasonable? I'm having some trouble determining what is a reasonable comment to make that isn't unnecessarily verbose, but still offers helpful documentation. https://codereview.chromium.org/342003005/diff/60001/chrome/browser/extension... File chrome/browser/extensions/unpacked_installer.cc (right): https://codereview.chromium.org/342003005/diff/60001/chrome/browser/extension... chrome/browser/extensions/unpacked_installer.cc:309: On 2014/06/27 22:31:08, Devlin wrote: > nit: remove extraneous newline. Done. https://codereview.chromium.org/342003005/diff/60001/chrome/browser/resources... File chrome/browser/resources/extensions/extension_load_error.html (right): https://codereview.chromium.org/342003005/diff/60001/chrome/browser/resources... chrome/browser/resources/extensions/extension_load_error.html:21: <div id="extension-load-error-additional"> On 2014/06/27 22:31:08, Devlin wrote: > these don't both need ids. Done. https://codereview.chromium.org/342003005/diff/60001/chrome/browser/resources... File chrome/browser/resources/extensions/extension_loader.js (right): https://codereview.chromium.org/342003005/diff/60001/chrome/browser/resources... chrome/browser/resources/extensions/extension_loader.js:40: this.additional_ = On 2014/06/27 22:31:08, Devlin wrote: > comments. Done. https://codereview.chromium.org/342003005/diff/60001/chrome/browser/resources... chrome/browser/resources/extensions/extension_loader.js:81: showAdditional: function(failures) { On 2014/06/27 22:31:08, Devlin wrote: > comments. Done. https://codereview.chromium.org/342003005/diff/60001/chrome/browser/resources... chrome/browser/resources/extensions/extension_loader.js:134: notifyAdditionalFailures: function(failures) { On 2014/06/27 22:31:08, Devlin wrote: > comments. Done. https://codereview.chromium.org/342003005/diff/60001/chrome/browser/resources... chrome/browser/resources/extensions/extension_loader.js:150: ExtensionLoader.notifyAdditional = function(failures) { On 2014/06/27 22:31:08, Devlin wrote: > comments. Done. https://codereview.chromium.org/342003005/diff/60001/chrome/browser/resources... File chrome/browser/resources/extensions/extensions.js (right): https://codereview.chromium.org/342003005/diff/60001/chrome/browser/resources... chrome/browser/resources/extensions/extensions.js:18: document.addEventListener('DOMContentLoaded', function() { Looks good! Done. https://codereview.chromium.org/342003005/diff/60001/chrome/browser/resources... chrome/browser/resources/extensions/extensions.js:19: chrome.send('extensionLoaderDisplayFailures'); On 2014/06/27 22:31:09, Devlin wrote: > nit: indentation. Done. https://codereview.chromium.org/342003005/diff/60001/chrome/browser/resources... chrome/browser/resources/extensions/extensions.js:22: window.addEventListener('beforeunload', function() { On 2014/06/27 22:31:09, Devlin wrote: > nit: indentation. Done. https://codereview.chromium.org/342003005/diff/60001/chrome/browser/ui/webui/... File chrome/browser/ui/webui/extensions/extension_loader_handler.cc (right): https://codereview.chromium.org/342003005/diff/60001/chrome/browser/ui/webui/... chrome/browser/ui/webui/extensions/extension_loader_handler.cc:134: public: On 2014/06/27 22:31:10, Devlin wrote: > fix indentation. Done. https://codereview.chromium.org/342003005/diff/60001/chrome/browser/ui/webui/... chrome/browser/ui/webui/extensions/extension_loader_handler.cc:135: explicit FailureData(base::FilePath path, std::string error, On 2014/06/27 22:31:10, Devlin wrote: > const & on non-simple arguments. Done. https://codereview.chromium.org/342003005/diff/60001/chrome/browser/ui/webui/... chrome/browser/ui/webui/extensions/extension_loader_handler.cc:135: explicit FailureData(base::FilePath path, std::string error, On 2014/06/27 22:31:09, Devlin wrote: > nit: one param per line. Done. https://codereview.chromium.org/342003005/diff/60001/chrome/browser/ui/webui/... chrome/browser/ui/webui/extensions/extension_loader_handler.cc:151: weak_ptr_factory_(this), On 2014/06/27 22:31:09, Devlin wrote: > weak_ptr_factory_ should be the final data member. Done. https://codereview.chromium.org/342003005/diff/60001/chrome/browser/ui/webui/... chrome/browser/ui/webui/extensions/extension_loader_handler.cc:219: if (!failures_.empty()) { On 2014/06/27 23:42:54, gpdavis wrote: > That is indeed much better. Will fix (y) Done. https://codereview.chromium.org/342003005/diff/60001/chrome/browser/ui/webui/... chrome/browser/ui/webui/extensions/extension_loader_handler.cc:220: FailureData *failure = failures_[0]; On 2014/06/27 22:31:09, Devlin wrote: > nit: FailureData* failure Done. https://codereview.chromium.org/342003005/diff/60001/chrome/browser/ui/webui/... chrome/browser/ui/webui/extensions/extension_loader_handler.cc:221: NotifyFrontendOfFailure(failure->file_path, failure->error, On 2014/06/27 22:31:09, Devlin wrote: > one arg per line. Done. https://codereview.chromium.org/342003005/diff/60001/chrome/browser/ui/webui/... chrome/browser/ui/webui/extensions/extension_loader_handler.cc:224: std::string list("Additional failures:"); Could you elaborate a little on this? I looked up i18n.h and I don't quite understand what it is. https://codereview.chromium.org/342003005/diff/60001/chrome/browser/ui/webui/... chrome/browser/ui/webui/extensions/extension_loader_handler.cc:225: for (size_t i = 1; i < failures_.size(); i++) { On 2014/06/27 22:31:09, Devlin wrote: > ++i. Done. https://codereview.chromium.org/342003005/diff/60001/chrome/browser/ui/webui/... chrome/browser/ui/webui/extensions/extension_loader_handler.cc:247: display_ready_ = true; On 2014/06/27 22:31:10, Devlin wrote: > This seems like an odd time to set |display_ready_|... Done. https://codereview.chromium.org/342003005/diff/60001/chrome/browser/ui/webui/... chrome/browser/ui/webui/extensions/extension_loader_handler.cc:308: display_ready_ = false; On 2014/06/27 23:42:54, gpdavis wrote: > This was initially done because I started off implementing this so that we never > assume the display is ready unless the flag was explicitly set before the call, > so I reset it beforehand. This is also the reason why it's set to true in > LoadUnpackedExtensionImple; if the user clicked the button to load the > extension, it follows that the UI must be ready. > > But since I added the events to the JS that notified this class when the UI was > ready or not ready, this is no longer necessary. I'll look into pulling these > out safely. Done. https://codereview.chromium.org/342003005/diff/60001/chrome/browser/ui/webui/... File chrome/browser/ui/webui/extensions/extension_loader_handler.h (right): https://codereview.chromium.org/342003005/diff/60001/chrome/browser/ui/webui/... chrome/browser/ui/webui/extensions/extension_loader_handler.h:52: std::vector<FailureData*> failures_; On 2014/06/27 22:31:10, Devlin wrote: > member variables at the bottom. Done. https://codereview.chromium.org/342003005/diff/60001/chrome/browser/ui/webui/... chrome/browser/ui/webui/extensions/extension_loader_handler.h:71: // Called when an unpacked extension fails to load. On 2014/06/27 22:31:10, Devlin wrote: > This comment should be updated to be: "ExtensionErrorReporter::Observer > implementation." Done.
Next bit of review social norms: If you publish and mail a bunch of comments with "done"s, it's best to upload a patch, too. Otherwise, I have nothing to look at to see all your hard work! :) Basically, the flow is: 1. Uploader posts review 2. Reviewer reviews, sends comments 3. Uploader either: * Responds with a question on a few of them, or * Uploads a new review with "done" on comments, or * Some combination of the above (e.g., "done" on 8/10 comments, a question on the other 2). 4. Reviewer reviews changes and answers questions (if any). 5. Repeat 3 and 4 until lg'd. We're perfectly understanding of WIP patches (just make a note), and prefer them to just having "done"s with no corresponding patch or questions with no code. The amount of space is takes to store a review on the server is cheap - don't be afraid to upload a bunch of patches. :) Enjoy your weekend! Cheers, - Devlin https://codereview.chromium.org/342003005/diff/60001/chrome/browser/ui/webui/... File chrome/browser/ui/webui/extensions/extension_loader_handler.cc (right): https://codereview.chromium.org/342003005/diff/60001/chrome/browser/ui/webui/... chrome/browser/ui/webui/extensions/extension_loader_handler.cc:142: base::FilePath file_path; On 2014/06/27 23:42:54, gpdavis wrote: > That was what I started with, but I wasn't sure if I could have a vector of > structs, so I made it a class instead. I suppose there should be no reason this > wouldn't work with structs. I'll have to play around with it and see if I can > get that switched over. A couple of C++ finer points: - A vector of structs is pretty easy. - You're not actually storing a vector of structs right now, you're storing a vector of pointers. - If you continue to store a vector of pointers (which is not bad!), you should add DISALLOW_COPY_AND_ASSIGN (even to a struct). But yeah - structs and classes in C++ are perfectly similar. The only real difference is that the default permission level for structs is public, whereas for classes it's private. https://codereview.chromium.org/342003005/diff/60001/chrome/browser/ui/webui/... chrome/browser/ui/webui/extensions/extension_loader_handler.cc:224: std::string list("Additional failures:"); On 2014/06/28 02:31:18, gpdavis wrote: > Could you elaborate a little on this? I looked up i18n.h and I don't quite > understand what it is. Ah, sorry :) i18n = internationalization. (See why? It's because there are 18 letters between the beginning 'i' and the final 'n' in the word 'internationalization'. We're so clever... ;)) Another common one is localization (l10n). Basically, this means that the string has to be a specialized variable that will be made substituted for the proper translation when displayed in the browser. For examples, see stuff like GetLocalizedValues(), l10n_util, etc. https://codereview.chromium.org/342003005/diff/60001/chrome/browser/ui/webui/... chrome/browser/ui/webui/extensions/extension_loader_handler.cc:228: list.append(failure->file_path.value()); On 2014/06/27 23:42:54, gpdavis wrote: > Sounds good. LossyDisplayName seem reasonable? Yep! https://codereview.chromium.org/342003005/diff/60001/chrome/browser/ui/webui/... chrome/browser/ui/webui/extensions/extension_loader_handler.cc:230: failures_.clear(); On 2014/06/27 23:42:54, gpdavis wrote: > I had a feeling there was going to be a memory leak here. > > What would be the preferred way to free up the memory of those references before > clearing the vector? For vectors, take a look at scoped_vector.h. Handy thing, that. :) https://codereview.chromium.org/342003005/diff/60001/chrome/browser/ui/webui/... chrome/browser/ui/webui/extensions/extension_loader_handler.cc:234: list_value); On 2014/06/27 23:42:54, gpdavis wrote: > This is certainly going to require heavier UI adjustments. Knowing that UI > modifications need to be specifically reviewed, I would like to have somebody > work with me to figure out the most reasonable solution. But we can cross that > bridge when we come to it. It would be a relatively minor adjustment to make > this send off all at once instead of the way it currently is. > > How about we make that determination once the primary architectural changes look > good? Sure, I'm not too worried about it. But the only parts that need to be UI reviewed are the visible changes - the suggestion was for mostly a logical change.
Thanks for the etiquette tips ;) I'll be a code review gentleman in no time! I uploaded a new patch set with the changes from all the "done"s from the last one, and I added some questions about design moving forward. https://codereview.chromium.org/342003005/diff/80001/chrome/browser/extension... File chrome/browser/extensions/extension_service.cc (right): https://codereview.chromium.org/342003005/diff/80001/chrome/browser/extension... chrome/browser/extensions/extension_service.cc:703: unpacked_installer->set_be_noisy_on_failure(false); Still need to figure out how to address this problem. Are we only displaying the new UI for explicit extension loads, reloads and chrome://extensions page refreshes? https://codereview.chromium.org/342003005/diff/80001/chrome/browser/ui/webui/... File chrome/browser/ui/webui/extensions/extension_loader_handler.cc (right): https://codereview.chromium.org/342003005/diff/80001/chrome/browser/ui/webui/... chrome/browser/ui/webui/extensions/extension_loader_handler.cc:212: base::string16 list = base::UTF8ToUTF16("Additional failures:"); Is this what you meant by internationalization? https://codereview.chromium.org/342003005/diff/80001/chrome/browser/ui/webui/... chrome/browser/ui/webui/extensions/extension_loader_handler.cc:221: "extensions.ExtensionLoader.notifyAdditional", I've been thinking about how to go about condensing this down to one message so that the UI can sort failures out on its own. There are only a few instances in which multiple failures are possible-- refreshing the extensions page, or explicitly reloading multiple broken extensions without handling the errors (by reloading or giving up). In the event of a page refresh, this should be relatively simple since everything necessary to build a list of broken extensions is set up. However, for multiple user triggered reloads, it gets tricky. I was thinking that we would need to keep track of all the currently active load failures somewhere, then we need to be able to send a message to the UI that another load has failed and that the UI needs to reflect that change without losing the information about the previous failures. Do you have any specific input on this? I think I could throw something together to make this work, but your feedback is always appreciated. https://codereview.chromium.org/342003005/diff/80001/chrome/browser/ui/webui/... File chrome/browser/ui/webui/extensions/extension_loader_handler.h (right): https://codereview.chromium.org/342003005/diff/80001/chrome/browser/ui/webui/... chrome/browser/ui/webui/extensions/extension_loader_handler.h:61: manifest(manifest) {} There should probably be a newline here, right? How's my struct look? :)
https://codereview.chromium.org/342003005/diff/80001/chrome/browser/extension... File chrome/browser/extensions/extension_error_reporter.h (right): https://codereview.chromium.org/342003005/diff/80001/chrome/browser/extension... chrome/browser/extensions/extension_error_reporter.h:35: // Observer function called when an unpacked extension fails to load. nit: simply "Called when..." is fine. Since it's in the Observer class, we can know it's an observer function :) https://codereview.chromium.org/342003005/diff/80001/chrome/browser/extension... File chrome/browser/extensions/extension_service.cc (right): https://codereview.chromium.org/342003005/diff/80001/chrome/browser/extension... chrome/browser/extensions/extension_service.cc:703: unpacked_installer->set_be_noisy_on_failure(false); On 2014/06/30 19:42:52, gpdavis wrote: > Still need to figure out how to address this problem. Are we only displaying > the new UI for explicit extension loads, reloads and chrome://extensions page > refreshes? Not even (as there can be explicit extension loads from an external source, like CAEDT). Basically, we only show the new UI for an extension load that was directly instigated by chrome:extensions. Ideally, we'll want to make the new UI present in external sources like CAEDT, too, but that's another project (possibly the next one). :) https://codereview.chromium.org/342003005/diff/80001/chrome/browser/resources... File chrome/browser/resources/extensions/extension_loader.js (right): https://codereview.chromium.org/342003005/diff/80001/chrome/browser/resources... chrome/browser/resources/extensions/extension_loader.js:9: window.addEventListener('beforeunload', function() { Actually, I'm wondering if this wouldn't be better on the C++ side. We don't typically use the low-level DOM functions like these (partly because we have complicated settings pages - frames in frames, etc). Could we put the ready signal inside the constructor for ExtensionLoader, and move the "not ready" to the C++ when learn the page is navigating? (This will be the same time that we reload all extensions for the chrome:extensions page). https://codereview.chromium.org/342003005/diff/80001/chrome/browser/resources... chrome/browser/resources/extensions/extension_loader.js:95: * Display additional load failures to the user. We should be able to clean this up when we move all of it into one message to send to the frontend. https://codereview.chromium.org/342003005/diff/80001/chrome/browser/ui/webui/... File chrome/browser/ui/webui/extensions/extension_loader_handler.cc (right): https://codereview.chromium.org/342003005/diff/80001/chrome/browser/ui/webui/... chrome/browser/ui/webui/extensions/extension_loader_handler.cc:212: base::string16 list = base::UTF8ToUTF16("Additional failures:"); On 2014/06/30 19:42:52, gpdavis wrote: > Is this what you meant by internationalization? Not quite. This changes the string from UTF8 to UTF16, which we _do_ want to do for most/all user-facing strings, but this doesn't actually internationalize it. You'll probably want to use something like l10n_util::GetStringUTF16() (codesearch has a ton of examples on it). That said, depending on the UI we use for the extra failures, we'll probably want to just put this in the html in the end. https://codereview.chromium.org/342003005/diff/80001/chrome/browser/ui/webui/... chrome/browser/ui/webui/extensions/extension_loader_handler.cc:214: FailureData *failure = failures_[i]; nit: FailureData* failure - note the asterisk position. https://codereview.chromium.org/342003005/diff/80001/chrome/browser/ui/webui/... chrome/browser/ui/webui/extensions/extension_loader_handler.cc:221: "extensions.ExtensionLoader.notifyAdditional", On 2014/06/30 19:42:52, gpdavis wrote: > I've been thinking about how to go about condensing this down to one message so > that the UI can sort failures out on its own. There are only a few instances in > which multiple failures are possible-- refreshing the extensions page, or > explicitly reloading multiple broken extensions without handling the errors (by > reloading or giving up). > > In the event of a page refresh, this should be relatively simple since > everything necessary to build a list of broken extensions is set up. However, > for multiple user triggered reloads, it gets tricky. I was thinking that we > would need to keep track of all the currently active load failures somewhere, > then we need to be able to send a message to the UI that another load has failed > and that the UI needs to reflect that change without losing the information > about the previous failures. > > Do you have any specific input on this? I think I could throw something > together to make this work, but your feedback is always appreciated. For this, at least as a starting point, I wouldn't mind only displaying load errors for the previous "batch" of loads. That is, if we refresh the page, display multiple load errors, but if the user does something like try to reload a second extension when one has already failed, just show the most recent. We don't have to persist a full list (and it might even be annoying if we did). That said, still waiting on an answer to what current behavior for multiple immediate load failures is (e.g., two load failures on page refresh)? This could determine how we want to make the new behavior look. https://codereview.chromium.org/342003005/diff/80001/chrome/browser/ui/webui/... chrome/browser/ui/webui/extensions/extension_loader_handler.cc:222: list_value); nit: inline the string value construction here. https://codereview.chromium.org/342003005/diff/80001/chrome/browser/ui/webui/... chrome/browser/ui/webui/extensions/extension_loader_handler.cc:277: failures_.push_back(failure); nit: inline the failuredata construction here. https://codereview.chromium.org/342003005/diff/80001/chrome/browser/ui/webui/... File chrome/browser/ui/webui/extensions/extension_loader_handler.h (right): https://codereview.chromium.org/342003005/diff/80001/chrome/browser/ui/webui/... chrome/browser/ui/webui/extensions/extension_loader_handler.h:54: explicit FailureData(base::FilePath file_path, no need for explicit with multiple parameters to the constructor (google "explicit constructor c++"). https://codereview.chromium.org/342003005/diff/80001/chrome/browser/ui/webui/... chrome/browser/ui/webui/extensions/extension_loader_handler.h:55: std::string error, nit: indentation error. https://codereview.chromium.org/342003005/diff/80001/chrome/browser/ui/webui/... chrome/browser/ui/webui/extensions/extension_loader_handler.h:61: manifest(manifest) {} Make an out-of-line constructor, and an explicit destructor. https://codereview.chromium.org/342003005/diff/80001/chrome/browser/ui/webui/... chrome/browser/ui/webui/extensions/extension_loader_handler.h:67: DISALLOW_COPY_AND_ASSIGN(FailureData); One note and one problem: note - we don't actually _need_ disallow copy and assign on this, because it's all simple stuff (and knows how to copy itself correctly). That said, it's always best to avoid it if we can, so just FYI that if you need to copy this, you can. :) problem - look at the definition of DISALLOW_COPY_AND_ASSIGN in base/macros.h - can you find why the way you have it won't work?
https://codereview.chromium.org/342003005/diff/80001/chrome/browser/extension... File chrome/browser/extensions/extension_error_reporter.h (right): https://codereview.chromium.org/342003005/diff/80001/chrome/browser/extension... chrome/browser/extensions/extension_error_reporter.h:35: // Observer function called when an unpacked extension fails to load. On 2014/06/30 21:06:41, Devlin wrote: > nit: simply "Called when..." is fine. Since it's in the Observer class, we can > know it's an observer function :) Done. https://codereview.chromium.org/342003005/diff/80001/chrome/browser/extension... File chrome/browser/extensions/extension_service.cc (right): https://codereview.chromium.org/342003005/diff/80001/chrome/browser/extension... chrome/browser/extensions/extension_service.cc:703: unpacked_installer->set_be_noisy_on_failure(false); On 2014/06/30 21:06:41, Devlin wrote: > On 2014/06/30 19:42:52, gpdavis wrote: > > Still need to figure out how to address this problem. Are we only displaying > > the new UI for explicit extension loads, reloads and chrome://extensions page > > refreshes? > > Not even (as there can be explicit extension loads from an external source, like > CAEDT). Basically, we only show the new UI for an extension load that was > directly instigated by chrome:extensions. > > Ideally, we'll want to make the new UI present in external sources like CAEDT, > too, but that's another project (possibly the next one). :) Where exactly is the relevant code that triggers the reloads of all the extensions when chrome://extensions is refreshed? I'm not sure where to send these signals from to indicate that the UI ought to be triggered. https://codereview.chromium.org/342003005/diff/80001/chrome/browser/resources... File chrome/browser/resources/extensions/extension_loader.js (right): https://codereview.chromium.org/342003005/diff/80001/chrome/browser/resources... chrome/browser/resources/extensions/extension_loader.js:9: window.addEventListener('beforeunload', function() { Will the ExtensionLoader be constructed after the DOM is ready? The problem I was running into was messages being sent before the page was ready to receive them (var extension didn't exist when the message was received). I wasn't sure how to determine if the page was loading from the C++ side. How would I go about doing that? https://codereview.chromium.org/342003005/diff/80001/chrome/browser/ui/webui/... File chrome/browser/ui/webui/extensions/extension_loader_handler.cc (right): https://codereview.chromium.org/342003005/diff/80001/chrome/browser/ui/webui/... chrome/browser/ui/webui/extensions/extension_loader_handler.cc:212: base::string16 list = base::UTF8ToUTF16("Additional failures:"); Wait-- I thought you had said this needed to be i18n, not l10n. I'm pretty confused about what exactly the point of this is. What does internationalizing a string do? If this were to remain in the C++ side (though I know with UI changes it likely won't), would I then need to create a string constant and an ID to pass into GetStringUTF16? https://codereview.chromium.org/342003005/diff/80001/chrome/browser/ui/webui/... chrome/browser/ui/webui/extensions/extension_loader_handler.cc:214: FailureData *failure = failures_[i]; On 2014/06/30 21:06:42, Devlin wrote: > nit: FailureData* failure - note the asterisk position. Done. https://codereview.chromium.org/342003005/diff/80001/chrome/browser/ui/webui/... chrome/browser/ui/webui/extensions/extension_loader_handler.cc:221: "extensions.ExtensionLoader.notifyAdditional", Oops-- thought I responded to that. Currently, multiple popups do show up, and they are overlapping. So, just to clarify, in this scenario, if the page were refreshed and multiple load errors were displayed, then a user reloaded one extension, the UI would revert to just showing the bad manifest of the one they reloaded, with no additional information about the load failures from the page refresh? https://codereview.chromium.org/342003005/diff/80001/chrome/browser/ui/webui/... chrome/browser/ui/webui/extensions/extension_loader_handler.cc:222: list_value); On 2014/06/30 21:06:42, Devlin wrote: > nit: inline the string value construction here. Done. https://codereview.chromium.org/342003005/diff/80001/chrome/browser/ui/webui/... chrome/browser/ui/webui/extensions/extension_loader_handler.cc:277: failures_.push_back(failure); On 2014/06/30 21:06:42, Devlin wrote: > nit: inline the failuredata construction here. Done. https://codereview.chromium.org/342003005/diff/80001/chrome/browser/ui/webui/... File chrome/browser/ui/webui/extensions/extension_loader_handler.h (right): https://codereview.chromium.org/342003005/diff/80001/chrome/browser/ui/webui/... chrome/browser/ui/webui/extensions/extension_loader_handler.h:54: explicit FailureData(base::FilePath file_path, On 2014/06/30 21:06:42, Devlin wrote: > no need for explicit with multiple parameters to the constructor (google > "explicit constructor c++"). Done. https://codereview.chromium.org/342003005/diff/80001/chrome/browser/ui/webui/... chrome/browser/ui/webui/extensions/extension_loader_handler.h:55: std::string error, On 2014/06/30 21:06:42, Devlin wrote: > nit: indentation error. Done. https://codereview.chromium.org/342003005/diff/80001/chrome/browser/ui/webui/... chrome/browser/ui/webui/extensions/extension_loader_handler.h:61: manifest(manifest) {} On 2014/06/30 21:06:42, Devlin wrote: > Make an out-of-line constructor, and an explicit destructor. An "out-of-line" constructor? I've never heard this term before. Can you explain? Done on the destructor, though. https://codereview.chromium.org/342003005/diff/80001/chrome/browser/ui/webui/... chrome/browser/ui/webui/extensions/extension_loader_handler.h:67: DISALLOW_COPY_AND_ASSIGN(FailureData); C++ is a pretty new language to me-- I'm pretty well familiar with C, but I started learning C++ at this job. I'm not really sure why that wouldn't work. I don't think I understand const and & well enough. Is there some reason you couldn't create a constructor for a struct with a reference to another instance of that struct? In any case, I'll remove it.
https://codereview.chromium.org/342003005/diff/80001/chrome/browser/extension... File chrome/browser/extensions/extension_service.cc (right): https://codereview.chromium.org/342003005/diff/80001/chrome/browser/extension... chrome/browser/extensions/extension_service.cc:703: unpacked_installer->set_be_noisy_on_failure(false); On 2014/06/30 21:41:19, gpdavis wrote: > On 2014/06/30 21:06:41, Devlin wrote: > > On 2014/06/30 19:42:52, gpdavis wrote: > > > Still need to figure out how to address this problem. Are we only > displaying > > > the new UI for explicit extension loads, reloads and chrome://extensions > page > > > refreshes? > > > > Not even (as there can be explicit extension loads from an external source, > like > > CAEDT). Basically, we only show the new UI for an extension load that was > > directly instigated by chrome:extensions. > > > > Ideally, we'll want to make the new UI present in external sources like CAEDT, > > too, but that's another project (possibly the next one). :) > > Where exactly is the relevant code that triggers the reloads of all the > extensions when chrome://extensions is refreshed? I'm not sure where to send > these signals from to indicate that the UI ought to be triggered. cs.chromium.org is pretty awesome. So, you know that the function to reload an extension is in ExtensionService, and is called ReloadExtension(). So we go there. https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ext... Then we look to see where it's called. https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ext... It looks like it's called from two places in WebUI, which sounds promising, since chrome:extensions is all WebUI. Let's check those two out. https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/... The first one doesn't seem very useful - it's just one extension. Let's look at the other. https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/... Bingo! Here we see it looping over all unpacked extensions and reloading them - that seems to be what we're looking for. So where's this one called from? https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/... Looks like it's called when the observed web contents has a provisional load. This makes sense with what we were thinking. Doing this kind of hunt through code is amazingly useful, and is good to practice. :) https://codereview.chromium.org/342003005/diff/80001/chrome/browser/resources... File chrome/browser/resources/extensions/extension_loader.js (right): https://codereview.chromium.org/342003005/diff/80001/chrome/browser/resources... chrome/browser/resources/extensions/extension_loader.js:9: window.addEventListener('beforeunload', function() { On 2014/06/30 21:41:19, gpdavis wrote: > Will the ExtensionLoader be constructed after the DOM is ready? The problem I > was running into was messages being sent before the page was ready to receive > them (var extension didn't exist when the message was received). > > I wasn't sure how to determine if the page was loading from the C++ side. How > would I go about doing that? Well, since extensions are loaded on page load, we can listen to that same event for when the page is probably loading, right? :) See the other comment for when that happens. I think that any messages sent from the constructor of ExtensionLoader will be sent after the extensions object is created, because ExtensionLoader is entirely within extensions. Give it a shot and see if it works. If not, we'll revisit it. https://codereview.chromium.org/342003005/diff/80001/chrome/browser/ui/webui/... File chrome/browser/ui/webui/extensions/extension_loader_handler.cc (right): https://codereview.chromium.org/342003005/diff/80001/chrome/browser/ui/webui/... chrome/browser/ui/webui/extensions/extension_loader_handler.cc:212: base::string16 list = base::UTF8ToUTF16("Additional failures:"); On 2014/06/30 21:41:19, gpdavis wrote: > Wait-- I thought you had said this needed to be i18n, not l10n. > > I'm pretty confused about what exactly the point of this is. What does > internationalizing a string do? If this were to remain in the C++ side (though > I know with UI changes it likely won't), would I then need to create a string > constant and an ID to pass into GetStringUTF16? Yeah, our libraries are funky. i18n and l10n are usually very similar, and I don't know if there's any difference in Chrome. For some reason, we usually still refer to it as i18n, though. So the general practice would be to add a new message to chrome/app/generated_resources.grd, and then use l10n_util::GetStringUTF16() to retrieve it. The messages in generated_resources.grd are translated into all the languages that chrome is shipped in - and l10n_util::GetStringUTF16() retrieves the string suited for the user's locale. Otherwise, every user would see the same message, even if it should be in a different language. https://codereview.chromium.org/342003005/diff/80001/chrome/browser/ui/webui/... chrome/browser/ui/webui/extensions/extension_loader_handler.cc:221: "extensions.ExtensionLoader.notifyAdditional", On 2014/06/30 21:41:19, gpdavis wrote: > Oops-- thought I responded to that. > > Currently, multiple popups do show up, and they are overlapping. > > So, just to clarify, in this scenario, if the page were refreshed and multiple > load errors were displayed, then a user reloaded one extension, the UI would > revert to just showing the bad manifest of the one they reloaded, with no > additional information about the load failures from the page refresh? Good point that it'd be somewhat strange for a user to see the errors disappear. That said, it's really such an edge case (the chances that multiple extensions will fail to reload is actually very low), so I think I'd be okay with it. Maybe just have the list be "And these other failures: " followed the full error message we currently give. In a followup, we can spruce up the UI to support multiple. https://codereview.chromium.org/342003005/diff/80001/chrome/browser/ui/webui/... File chrome/browser/ui/webui/extensions/extension_loader_handler.h (right): https://codereview.chromium.org/342003005/diff/80001/chrome/browser/ui/webui/... chrome/browser/ui/webui/extensions/extension_loader_handler.h:61: manifest(manifest) {} On 2014/06/30 21:41:19, gpdavis wrote: > On 2014/06/30 21:06:42, Devlin wrote: > > Make an out-of-line constructor, and an explicit destructor. > > An "out-of-line" constructor? I've never heard this term before. Can you > explain? > > Done on the destructor, though. out-of-line is the (slang) opposite of inline. Meaning, put the definition outside of the class definition. https://codereview.chromium.org/342003005/diff/80001/chrome/browser/ui/webui/... chrome/browser/ui/webui/extensions/extension_loader_handler.h:67: DISALLOW_COPY_AND_ASSIGN(FailureData); On 2014/06/30 21:41:19, gpdavis wrote: > C++ is a pretty new language to me-- I'm pretty well familiar with C, but I > started learning C++ at this job. > > I'm not really sure why that wouldn't work. I don't think I understand const > and & well enough. Is there some reason you couldn't create a constructor for a > struct with a reference to another instance of that struct? > > In any case, I'll remove it. So, what DISALLOW_COPY_AND_ASSIGN does is just declares the two "missing" constructors (copy constructor and assignment constructor). The way this normally works is by doing: class Foo { public: Foo(); ~Foo(); void Bar(); private: DISALLOW_COPY_AND_ASSIGN(Foo); } This "disallows" the two other constructors because they are in the classes private namespace - no outside callers can call them, and it will result in a compile error. By putting a DISALLOW_COPY_AND_ASSIGN in the public namespace of the struct, it does not restrict other classes from calling them. Make sense?
https://codereview.chromium.org/342003005/diff/80001/chrome/browser/extension... File chrome/browser/extensions/extension_service.cc (right): https://codereview.chromium.org/342003005/diff/80001/chrome/browser/extension... chrome/browser/extensions/extension_service.cc:703: unpacked_installer->set_be_noisy_on_failure(false); I am fairly well acquainted with code search (I certainly wouldn't have made it this far without it!), but I was going down that rabbit hole a little while back and got lost rather quickly. I think now that I'm more familiar with the relevant code, I probably could have found that. Thanks for the pointer in the right direction, though! So we need some way to set the noisy flag in the installer to false. How about adding a parameter to ReloadExtension, as I suggested earlier? Seems like a reasonable thing to require for such a function call. Or maybe overloading it so that any calls without the boolean flag will be redirected with a default flag value? Something like this: ReloadExtension(const std::string& transient_extension_id, bool be_noisy) { //current reload code, implementing boolean } ReloadExtension(const std::string& transient_extension_id) { ReloadExtension(transient_extension_id, true); } https://codereview.chromium.org/342003005/diff/80001/chrome/browser/resources... File chrome/browser/resources/extensions/extension_loader.js (right): https://codereview.chromium.org/342003005/diff/80001/chrome/browser/resources... chrome/browser/resources/extensions/extension_loader.js:9: window.addEventListener('beforeunload', function() { Wait, are you talking about the Extension Loader Handler constructor? Because that object is constructed on a page load, but isn't destroyed and reconstructed on a refresh. Also, I should point out that extensions are not reloaded on chrome://extensions page loads, only reloads. So if the manifest is broken between starting up chrome and loading the extensions page, it won't show up until a page refresh of chrome://extensions, or an explicit reload of the extensions. https://codereview.chromium.org/342003005/diff/80001/chrome/browser/ui/webui/... File chrome/browser/ui/webui/extensions/extension_loader_handler.cc (right): https://codereview.chromium.org/342003005/diff/80001/chrome/browser/ui/webui/... chrome/browser/ui/webui/extensions/extension_loader_handler.cc:212: base::string16 list = base::UTF8ToUTF16("Additional failures:"); Ah, okay, that makes sense. But since we're outsourcing this string population to the JS, I won't add it in. It's good to know how that works, though. In what particular instances is internationalization a critical thing? Anything that's user facing? https://codereview.chromium.org/342003005/diff/80001/chrome/browser/ui/webui/... chrome/browser/ui/webui/extensions/extension_loader_handler.cc:221: "extensions.ExtensionLoader.notifyAdditional", Yeah, you're right, that is quite an edge case. It seems to me the UI ought to behave like a stack; the first load error shows the manifest, then the next error will display the manifest for the next load failure and push the old one down to a minimized version (ie, other load failure: |path|). Then if the displayed error is dealt with, the one that was pushed down gets popped back up with its manifest error displayed. This may be a challenge to implement in the JS, but I'm up for the challenge. Let me know if you've got any other input on this. If not, I'll just take a stab at it and have you pick apart my solution ;) https://codereview.chromium.org/342003005/diff/80001/chrome/browser/ui/webui/... File chrome/browser/ui/webui/extensions/extension_loader_handler.h (right): https://codereview.chromium.org/342003005/diff/80001/chrome/browser/ui/webui/... chrome/browser/ui/webui/extensions/extension_loader_handler.h:61: manifest(manifest) {} Ah, gotcha. Can do. https://codereview.chromium.org/342003005/diff/80001/chrome/browser/ui/webui/... chrome/browser/ui/webui/extensions/extension_loader_handler.h:67: DISALLOW_COPY_AND_ASSIGN(FailureData); Ahh, I see. So by creating private dummies for those constructors, there's no way for them to be accessed. That does make sense. Thanks for that explanation!
https://codereview.chromium.org/342003005/diff/80001/chrome/browser/extension... File chrome/browser/extensions/extension_service.cc (right): https://codereview.chromium.org/342003005/diff/80001/chrome/browser/extension... chrome/browser/extensions/extension_service.cc:703: unpacked_installer->set_be_noisy_on_failure(false); On 2014/06/30 23:35:11, gpdavis wrote: > I am fairly well acquainted with code search (I certainly wouldn't have made it > this far without it!), but I was going down that rabbit hole a little while back > and got lost rather quickly. I think now that I'm more familiar with the > relevant code, I probably could have found that. Thanks for the pointer in the > right direction, though! > > So we need some way to set the noisy flag in the installer to false. How about > adding a parameter to ReloadExtension, as I suggested earlier? Seems like a > reasonable thing to require for such a function call. Or maybe overloading it > so that any calls without the boolean flag will be redirected with a default > flag value? Something like this: > > ReloadExtension(const std::string& transient_extension_id, bool be_noisy) { > //current reload code, implementing boolean > } > > ReloadExtension(const std::string& transient_extension_id) { > ReloadExtension(transient_extension_id, true); > } Yeah, adding a boolean be_noisy to ExtensionService is fine. IIRC, your previous suggestion was to have ExtensionService call the WebUI on failure, which is incorrect (as it shouldn't have any knowledge of it). If possible, prefer only one Reload method (as opposed to overloading). https://codereview.chromium.org/342003005/diff/80001/chrome/browser/resources... File chrome/browser/resources/extensions/extension_loader.js (right): https://codereview.chromium.org/342003005/diff/80001/chrome/browser/resources... chrome/browser/resources/extensions/extension_loader.js:9: window.addEventListener('beforeunload', function() { On 2014/06/30 23:35:11, gpdavis wrote: > Wait, are you talking about the Extension Loader Handler constructor? Because > that object is constructed on a page load, but isn't destroyed and reconstructed > on a refresh. I was talking about the ExtensionLoader JS class. > Also, I should point out that extensions are not reloaded on chrome://extensions > page loads, only reloads. So if the manifest is broken between starting up > chrome and loading the extensions page, it won't show up until a page refresh of > chrome://extensions, or an explicit reload of the extensions. That should be fine - for now, let's worry about matching current behavior with the new UI. https://codereview.chromium.org/342003005/diff/80001/chrome/browser/ui/webui/... File chrome/browser/ui/webui/extensions/extension_loader_handler.cc (right): https://codereview.chromium.org/342003005/diff/80001/chrome/browser/ui/webui/... chrome/browser/ui/webui/extensions/extension_loader_handler.cc:212: base::string16 list = base::UTF8ToUTF16("Additional failures:"); On 2014/06/30 23:35:11, gpdavis wrote: > Ah, okay, that makes sense. But since we're outsourcing this string population > to the JS, I won't add it in. It's good to know how that works, though. In > what particular instances is internationalization a critical thing? Anything > that's user facing? _almost_ anything that's user facing. There are a few exceptions for things like extension manifest errors, and we did that so that developers could google the error and get help. But any part of the UI like this should be. https://codereview.chromium.org/342003005/diff/80001/chrome/browser/ui/webui/... chrome/browser/ui/webui/extensions/extension_loader_handler.cc:221: "extensions.ExtensionLoader.notifyAdditional", On 2014/06/30 23:35:11, gpdavis wrote: > Yeah, you're right, that is quite an edge case. It seems to me the UI ought to > behave like a stack; the first load error shows the manifest, then the next > error will display the manifest for the next load failure and push the old one > down to a minimized version (ie, other load failure: |path|). Then if the > displayed error is dealt with, the one that was pushed down gets popped back up > with its manifest error displayed. > > This may be a challenge to implement in the JS, but I'm up for the challenge. > Let me know if you've got any other input on this. If not, I'll just take a > stab at it and have you pick apart my solution ;) I would envision something similar, though with more of an accordion UI. If you wanna take it on, that'd be great. :) But let's get this patch in first, and do that in a followup.
https://codereview.chromium.org/342003005/diff/80001/chrome/browser/extension... File chrome/browser/extensions/extension_service.cc (right): https://codereview.chromium.org/342003005/diff/80001/chrome/browser/extension... chrome/browser/extensions/extension_service.cc:703: unpacked_installer->set_be_noisy_on_failure(false); On 2014/07/01 17:02:55, Devlin wrote: > On 2014/06/30 23:35:11, gpdavis wrote: > > I am fairly well acquainted with code search (I certainly wouldn't have made > it > > this far without it!), but I was going down that rabbit hole a little while > back > > and got lost rather quickly. I think now that I'm more familiar with the > > relevant code, I probably could have found that. Thanks for the pointer in > the > > right direction, though! > > > > So we need some way to set the noisy flag in the installer to false. How > about > > adding a parameter to ReloadExtension, as I suggested earlier? Seems like a > > reasonable thing to require for such a function call. Or maybe overloading it > > so that any calls without the boolean flag will be redirected with a default > > flag value? Something like this: > > > > ReloadExtension(const std::string& transient_extension_id, bool be_noisy) { > > //current reload code, implementing boolean > > } > > > > ReloadExtension(const std::string& transient_extension_id) { > > ReloadExtension(transient_extension_id, true); > > } > > Yeah, adding a boolean be_noisy to ExtensionService is fine. IIRC, your > previous suggestion was to have ExtensionService call the WebUI on failure, > which is incorrect (as it shouldn't have any knowledge of it). If possible, > prefer only one Reload method (as opposed to overloading). Sweet. I went ahead and added that argument and updated all calls to the function instead of overloading. https://codereview.chromium.org/342003005/diff/80001/chrome/browser/resources... File chrome/browser/resources/extensions/extension_loader.js (right): https://codereview.chromium.org/342003005/diff/80001/chrome/browser/resources... chrome/browser/resources/extensions/extension_loader.js:9: window.addEventListener('beforeunload', function() { On 2014/07/01 17:02:55, Devlin wrote: > On 2014/06/30 23:35:11, gpdavis wrote: > > Wait, are you talking about the Extension Loader Handler constructor? Because > > that object is constructed on a page load, but isn't destroyed and > reconstructed > > on a refresh. > > I was talking about the ExtensionLoader JS class. > > > > Also, I should point out that extensions are not reloaded on > chrome://extensions > > page loads, only reloads. So if the manifest is broken between starting up > > chrome and loading the extensions page, it won't show up until a page refresh > of > > chrome://extensions, or an explicit reload of the extensions. > > That should be fine - for now, let's worry about matching current behavior with > the new UI. I replaced the DOMContentLoaded listener with a message sent at the end of init, and it still works. I'm not sure what to do about the beforeunload listener, though. Perhaps a GetInstanceOf method in the Extension Loader Handler would make this a simple matter of getting the instance and making a function call that will signal that the display isn't ready? https://codereview.chromium.org/342003005/diff/80001/chrome/browser/ui/webui/... File chrome/browser/ui/webui/extensions/extension_loader_handler.cc (right): https://codereview.chromium.org/342003005/diff/80001/chrome/browser/ui/webui/... chrome/browser/ui/webui/extensions/extension_loader_handler.cc:221: "extensions.ExtensionLoader.notifyAdditional", On 2014/07/01 17:02:55, Devlin wrote: > On 2014/06/30 23:35:11, gpdavis wrote: > > Yeah, you're right, that is quite an edge case. It seems to me the UI ought > to > > behave like a stack; the first load error shows the manifest, then the next > > error will display the manifest for the next load failure and push the old one > > down to a minimized version (ie, other load failure: |path|). Then if the > > displayed error is dealt with, the one that was pushed down gets popped back > up > > with its manifest error displayed. > > > > This may be a challenge to implement in the JS, but I'm up for the challenge. > > Let me know if you've got any other input on this. If not, I'll just take a > > stab at it and have you pick apart my solution ;) > > I would envision something similar, though with more of an accordion UI. If you > wanna take it on, that'd be great. :) But let's get this patch in first, and do > that in a followup. I was already mostly done with this last night before you responded, so I uploaded my changes. I'm eager to hear what you think!
https://codereview.chromium.org/342003005/diff/80001/chrome/browser/resources... File chrome/browser/resources/extensions/extension_loader.js (right): https://codereview.chromium.org/342003005/diff/80001/chrome/browser/resources... chrome/browser/resources/extensions/extension_loader.js:9: window.addEventListener('beforeunload', function() { On 2014/07/01 20:58:55, gpdavis wrote: > On 2014/07/01 17:02:55, Devlin wrote: > > On 2014/06/30 23:35:11, gpdavis wrote: > > > Wait, are you talking about the Extension Loader Handler constructor? > Because > > > that object is constructed on a page load, but isn't destroyed and > > reconstructed > > > on a refresh. > > > > I was talking about the ExtensionLoader JS class. > > > > > > > Also, I should point out that extensions are not reloaded on > > chrome://extensions > > > page loads, only reloads. So if the manifest is broken between starting up > > > chrome and loading the extensions page, it won't show up until a page > refresh > > of > > > chrome://extensions, or an explicit reload of the extensions. > > > > That should be fine - for now, let's worry about matching current behavior > with > > the new UI. > > I replaced the DOMContentLoaded listener with a message sent at the end of init, > and it still works. I'm not sure what to do about the beforeunload listener, > though. Perhaps a GetInstanceOf method in the Extension Loader Handler would > make this a simple matter of getting the instance and making a function call > that will signal that the display isn't ready? Can we not just unset it when the page is reloading, and reset it when it's recreated? https://codereview.chromium.org/342003005/diff/140001/apps/app_load_service.cc File apps/app_load_service.cc (right): https://codereview.chromium.org/342003005/diff/140001/apps/app_load_service.c... apps/app_load_service.cc:50: service->ReloadExtension(extension_id, true); document anonymous bool. https://codereview.chromium.org/342003005/diff/140001/chrome/browser/extensio... File chrome/browser/extensions/extension_service.h (right): https://codereview.chromium.org/342003005/diff/140001/chrome/browser/extensio... chrome/browser/extensions/extension_service.h:240: void ReloadExtension(const std::string& extension_id, bool be_noisy); document be_noisy's behavior. https://codereview.chromium.org/342003005/diff/140001/chrome/browser/resource... File chrome/browser/resources/extensions/extension_loader.js (right): https://codereview.chromium.org/342003005/diff/140001/chrome/browser/resource... chrome/browser/resources/extensions/extension_loader.js:61: * @type {Array} Array of whats? (document) https://codereview.chromium.org/342003005/diff/140001/chrome/browser/resource... chrome/browser/resources/extensions/extension_loader.js:69: [this.failures_[this.failures_.length - 1].path_]); We can't do this. We deliberately do not allow WebUI to choose a file path to load as a security measure. File selection must be done in C++ only. https://codereview.chromium.org/342003005/diff/140001/chrome/browser/resource... chrome/browser/resources/extensions/extension_loader.js:82: * Add a failure to failures_ array. If there is already a displayed nit: one space after a period (Clint will never forget this one ;)) https://codereview.chromium.org/342003005/diff/140001/chrome/browser/resource... chrome/browser/resources/extensions/extension_loader.js:89: if (this.failures_.length == 1) { nit: no brackets around single-line if https://codereview.chromium.org/342003005/diff/140001/chrome/browser/resource... chrome/browser/resources/extensions/extension_loader.js:113: if (this.failures_.length == 1) { nit: brackets. https://codereview.chromium.org/342003005/diff/140001/chrome/browser/resource... chrome/browser/resources/extensions/extension_loader.js:122: * and error displayed, while additional failures have their path names I don't think we wrap like this for these comments... are you copying style from elsewhere? https://codereview.chromium.org/342003005/diff/140001/chrome/browser/resource... chrome/browser/resources/extensions/extension_loader.js:138: this.additional_.textContent = 'Additional failures:'; this will still have to be i18n'd. ;) https://codereview.chromium.org/342003005/diff/140001/chrome/browser/resource... chrome/browser/resources/extensions/extension_loader.js:140: this.additional_.textContent += '\n' + this.failures_[i].path_; nit: no brackets around 1-line for loops https://codereview.chromium.org/342003005/diff/140001/chrome/browser/ui/webui... File chrome/browser/ui/webui/extensions/extension_loader_handler.h (right): https://codereview.chromium.org/342003005/diff/140001/chrome/browser/ui/webui... chrome/browser/ui/webui/extensions/extension_loader_handler.h:9: #include <vector> Don't need this now, right? https://codereview.chromium.org/342003005/diff/140001/chrome/browser/ui/webui... chrome/browser/ui/webui/extensions/extension_loader_handler.h:53: struct FailureData { Can we actually move the definition to the .cc file and just forward-declare it here? https://codereview.chromium.org/342003005/diff/140001/chrome/browser/ui/webui... chrome/browser/ui/webui/extensions/extension_loader_handler.h:59: ~FailureData() {} For a class with member data, we actually prefer the destructor to be out-of-line, too.
https://codereview.chromium.org/342003005/diff/140001/apps/app_load_service.cc File apps/app_load_service.cc (right): https://codereview.chromium.org/342003005/diff/140001/apps/app_load_service.c... apps/app_load_service.cc:50: service->ReloadExtension(extension_id, true); On 2014/07/01 22:14:17, Devlin wrote: > document anonymous bool. Done. https://codereview.chromium.org/342003005/diff/140001/chrome/browser/extensio... File chrome/browser/extensions/extension_service.h (right): https://codereview.chromium.org/342003005/diff/140001/chrome/browser/extensio... chrome/browser/extensions/extension_service.h:240: void ReloadExtension(const std::string& extension_id, bool be_noisy); On 2014/07/01 22:14:17, Devlin wrote: > document be_noisy's behavior. Done. https://codereview.chromium.org/342003005/diff/140001/chrome/browser/resource... File chrome/browser/resources/extensions/extension_loader.js (right): https://codereview.chromium.org/342003005/diff/140001/chrome/browser/resource... chrome/browser/resources/extensions/extension_loader.js:61: * @type {Array} On 2014/07/01 22:14:17, Devlin wrote: > Array of whats? (document) An array of failures! Just kidding. Added in a Failure constructor and capitalized Failures in the comment to be more precise. https://codereview.chromium.org/342003005/diff/140001/chrome/browser/resource... chrome/browser/resources/extensions/extension_loader.js:69: [this.failures_[this.failures_.length - 1].path_]); On 2014/07/01 22:14:17, Devlin wrote: > We can't do this. We deliberately do not allow WebUI to choose a file path to > load as a security measure. File selection must be done in C++ only. Rats. I suppose I'll add a vector of file paths and an OnLoadSuccess to the observer so that the cc file knows what path to load when it receives the message. https://codereview.chromium.org/342003005/diff/140001/chrome/browser/resource... chrome/browser/resources/extensions/extension_loader.js:82: * Add a failure to failures_ array. If there is already a displayed On 2014/07/01 22:14:17, Devlin wrote: > nit: one space after a period (Clint will never forget this one ;)) He most certainly has not ;) Done. https://codereview.chromium.org/342003005/diff/140001/chrome/browser/resource... chrome/browser/resources/extensions/extension_loader.js:89: if (this.failures_.length == 1) { On 2014/07/01 22:14:17, Devlin wrote: > nit: no brackets around single-line if Done. https://codereview.chromium.org/342003005/diff/140001/chrome/browser/resource... chrome/browser/resources/extensions/extension_loader.js:113: if (this.failures_.length == 1) { On 2014/07/01 22:14:17, Devlin wrote: > nit: brackets. Done. https://codereview.chromium.org/342003005/diff/140001/chrome/browser/resource... chrome/browser/resources/extensions/extension_loader.js:122: * and error displayed, while additional failures have their path names On 2014/07/01 22:14:17, Devlin wrote: > I don't think we wrap like this for these comments... are you copying style from > elsewhere? See line 176. Should I do it a different way? https://codereview.chromium.org/342003005/diff/140001/chrome/browser/resource... chrome/browser/resources/extensions/extension_loader.js:138: this.additional_.textContent = 'Additional failures:'; On 2014/07/01 22:14:17, Devlin wrote: > this will still have to be i18n'd. ;) How do in JS? https://codereview.chromium.org/342003005/diff/140001/chrome/browser/resource... chrome/browser/resources/extensions/extension_loader.js:140: this.additional_.textContent += '\n' + this.failures_[i].path_; On 2014/07/01 22:14:17, Devlin wrote: > nit: no brackets around 1-line for loops Done. https://codereview.chromium.org/342003005/diff/140001/chrome/browser/ui/webui... File chrome/browser/ui/webui/extensions/extension_loader_handler.h (right): https://codereview.chromium.org/342003005/diff/140001/chrome/browser/ui/webui... chrome/browser/ui/webui/extensions/extension_loader_handler.h:9: #include <vector> On 2014/07/01 22:14:17, Devlin wrote: > Don't need this now, right? I will when I add in a vector of file paths since they can't be sent with the reload message ;) https://codereview.chromium.org/342003005/diff/140001/chrome/browser/ui/webui... chrome/browser/ui/webui/extensions/extension_loader_handler.h:53: struct FailureData { On 2014/07/01 22:14:17, Devlin wrote: > Can we actually move the definition to the .cc file and just forward-declare it > here? Yessir we can. https://codereview.chromium.org/342003005/diff/140001/chrome/browser/ui/webui... chrome/browser/ui/webui/extensions/extension_loader_handler.h:59: ~FailureData() {} On 2014/07/01 22:14:17, Devlin wrote: > For a class with member data, we actually prefer the destructor to be > out-of-line, too. So if these are out of line, do they still go outside the declaration in the cc file? struct FailureData { FailureData(... ...) : ...(...) {} ... } or struct FailureData { FailureData(... ...) ... } FailureData::FailureData(..., ...) : ...(...) {}
https://codereview.chromium.org/342003005/diff/80001/chrome/browser/resources... File chrome/browser/resources/extensions/extension_loader.js (right): https://codereview.chromium.org/342003005/diff/80001/chrome/browser/resources... chrome/browser/resources/extensions/extension_loader.js:9: window.addEventListener('beforeunload', function() { On 2014/07/01 22:14:17, Devlin wrote: > On 2014/07/01 20:58:55, gpdavis wrote: > > On 2014/07/01 17:02:55, Devlin wrote: > > > On 2014/06/30 23:35:11, gpdavis wrote: > > > > Wait, are you talking about the Extension Loader Handler constructor? > > Because > > > > that object is constructed on a page load, but isn't destroyed and > > > reconstructed > > > > on a refresh. > > > > > > I was talking about the ExtensionLoader JS class. > > > > > > > > > > Also, I should point out that extensions are not reloaded on > > > chrome://extensions > > > > page loads, only reloads. So if the manifest is broken between starting > up > > > > chrome and loading the extensions page, it won't show up until a page > > refresh > > > of > > > > chrome://extensions, or an explicit reload of the extensions. > > > > > > That should be fine - for now, let's worry about matching current behavior > > with > > > the new UI. > > > > I replaced the DOMContentLoaded listener with a message sent at the end of > init, > > and it still works. I'm not sure what to do about the beforeunload listener, > > though. Perhaps a GetInstanceOf method in the Extension Loader Handler would > > make this a simple matter of getting the instance and making a function call > > that will signal that the display isn't ready? > > Can we not just unset it when the page is reloading, and reset it when it's > recreated? Well, that is the goal. I figured adding a listener here would be the most direct way to ensure that it's set to false when the page begins reloading. Not sure where else to put it, unless it's put in the C++ where all the extensions are reloaded, which would require an instance getter, as I suggested. Unless I'm missing something fundamental, here, which is most certainly possible.
https://codereview.chromium.org/342003005/diff/140001/chrome/browser/resource... File chrome/browser/resources/extensions/extension_loader.js (right): https://codereview.chromium.org/342003005/diff/140001/chrome/browser/resource... chrome/browser/resources/extensions/extension_loader.js:138: this.additional_.textContent = 'Additional failures:'; On 2014/07/01 23:48:10, gpdavis wrote: > On 2014/07/01 22:14:17, Devlin wrote: > > this will still have to be i18n'd. ;) > > How do in JS? This was a premature request for help. Got it figured out (I think).
https://codereview.chromium.org/342003005/diff/80001/chrome/browser/resources... File chrome/browser/resources/extensions/extension_loader.js (right): https://codereview.chromium.org/342003005/diff/80001/chrome/browser/resources... chrome/browser/resources/extensions/extension_loader.js:9: window.addEventListener('beforeunload', function() { On 2014/07/01 23:50:59, gpdavis wrote: > On 2014/07/01 22:14:17, Devlin wrote: > > On 2014/07/01 20:58:55, gpdavis wrote: > > > On 2014/07/01 17:02:55, Devlin wrote: > > > > On 2014/06/30 23:35:11, gpdavis wrote: > > > > > Wait, are you talking about the Extension Loader Handler constructor? > > > Because > > > > > that object is constructed on a page load, but isn't destroyed and > > > > reconstructed > > > > > on a refresh. > > > > > > > > I was talking about the ExtensionLoader JS class. > > > > > > > > > > > > > Also, I should point out that extensions are not reloaded on > > > > chrome://extensions > > > > > page loads, only reloads. So if the manifest is broken between starting > > up > > > > > chrome and loading the extensions page, it won't show up until a page > > > refresh > > > > of > > > > > chrome://extensions, or an explicit reload of the extensions. > > > > > > > > That should be fine - for now, let's worry about matching current behavior > > > with > > > > the new UI. > > > > > > I replaced the DOMContentLoaded listener with a message sent at the end of > > init, > > > and it still works. I'm not sure what to do about the beforeunload > listener, > > > though. Perhaps a GetInstanceOf method in the Extension Loader Handler > would > > > make this a simple matter of getting the instance and making a function call > > > that will signal that the display isn't ready? > > > > Can we not just unset it when the page is reloading, and reset it when it's > > recreated? > > Well, that is the goal. I figured adding a listener here would be the most > direct way to ensure that it's set to false when the page begins reloading. Not > sure where else to put it, unless it's put in the C++ where all the extensions > are reloaded, which would require an instance getter, as I suggested. > > Unless I'm missing something fundamental, here, which is most certainly > possible. Can we not just listen for the same signal that ExtensionSettingsHandler does for its reload? https://codereview.chromium.org/342003005/diff/140001/chrome/browser/resource... File chrome/browser/resources/extensions/extension_loader.js (right): https://codereview.chromium.org/342003005/diff/140001/chrome/browser/resource... chrome/browser/resources/extensions/extension_loader.js:122: * and error displayed, while additional failures have their path names On 2014/07/01 23:48:10, gpdavis wrote: > On 2014/07/01 22:14:17, Devlin wrote: > > I don't think we wrap like this for these comments... are you copying style > from > > elsewhere? > > See line 176. Should I do it a different way? I think that we only line-wrap for param declarations, etc, not for the full function comment. https://codereview.chromium.org/342003005/diff/140001/chrome/browser/ui/webui... File chrome/browser/ui/webui/extensions/extension_loader_handler.h (right): https://codereview.chromium.org/342003005/diff/140001/chrome/browser/ui/webui... chrome/browser/ui/webui/extensions/extension_loader_handler.h:59: ~FailureData() {} On 2014/07/01 23:48:10, gpdavis wrote: > On 2014/07/01 22:14:17, Devlin wrote: > > For a class with member data, we actually prefer the destructor to be > > out-of-line, too. > > So if these are out of line, do they still go outside the declaration in the cc > file? > > struct FailureData { > FailureData(... > ...) > : ...(...) {} > ... > } > > or > > struct FailureData { > FailureData(... > ...) > .../ > } > > FailureData::FailureData(..., > ...) > : ...(...) {} The latter. inline wrt classes basically means "declared and defined simultaneously in the same piece of code." https://codereview.chromium.org/342003005/diff/180001/apps/app_load_service.cc File apps/app_load_service.cc (right): https://codereview.chromium.org/342003005/diff/180001/apps/app_load_service.c... apps/app_load_service.cc:50: // Reload the extension and allow noisy notifications. Nice, but a little verbose. Simply service->ReloadExtension(extension_id, true /* be noisy */); is sufficient. https://codereview.chromium.org/342003005/diff/180001/chrome/app/generated_re... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/342003005/diff/180001/chrome/app/generated_re... chrome/app/generated_resources.grd:4765: + <message name="IDS_EXTENSIONS_LOAD_ADDITIONAL_FAILURES" desc="The text displayed above additional failures."> "...additional extension load failures" https://codereview.chromium.org/342003005/diff/180001/chrome/browser/backgrou... File chrome/browser/background/background_contents_service.cc (right): https://codereview.chromium.org/342003005/diff/180001/chrome/browser/backgrou... chrome/browser/background/background_contents_service.cc:132: ReloadExtension(copied_extension_id, true); all of these should have anonymous bool comments... On second thought.... I'm beginning to doubt whether or not we should force every caller to do this. It creates a bit more noise in the CL than I was expecting. Let's go back to making two functions, ReloadExtension() and ReloadExtensionWithQuietFailure(). Sorry for the extra work - I should have looked at all the call sites before I requested it :/ https://codereview.chromium.org/342003005/diff/180001/chrome/browser/resource... File chrome/browser/resources/extensions/extension_loader.js (right): https://codereview.chromium.org/342003005/diff/180001/chrome/browser/resource... chrome/browser/resources/extensions/extension_loader.js:31: this.path_ = filePath; no trailing _ if it's not a private var. https://codereview.chromium.org/342003005/diff/180001/chrome/browser/resource... chrome/browser/resources/extensions/extension_loader.js:74: * @type {Array} There's a syntax for saying what the array is full of, I think. Check on the closure compiler docs or look around the source code more. https://codereview.chromium.org/342003005/diff/180001/chrome/browser/resource... chrome/browser/resources/extensions/extension_loader.js:103: this.additional_.hidden = false; Let's put the logic for showing/hidding |additional| in show(). https://codereview.chromium.org/342003005/diff/180001/chrome/browser/ui/webui... File chrome/browser/ui/webui/extensions/extension_loader_handler.cc (right): https://codereview.chromium.org/342003005/diff/180001/chrome/browser/ui/webui... chrome/browser/ui/webui/extensions/extension_loader_handler.cc:134: FailureData(base::FilePath file_path, this should take const file path and string. https://codereview.chromium.org/342003005/diff/180001/chrome/browser/ui/webui... chrome/browser/ui/webui/extensions/extension_loader_handler.cc:152: ExtensionLoaderHandler::FailureData::FailureData(base::FilePath file_path, prefer this one :) https://codereview.chromium.org/342003005/diff/180001/chrome/browser/ui/webui... chrome/browser/ui/webui/extensions/extension_loader_handler.cc:208: base::Bind(&ExtensionLoaderHandler::HandleGiveUp, Even though we call it give up in the text, let's change this to what it actually is - ignore failure. https://codereview.chromium.org/342003005/diff/180001/chrome/browser/ui/webui... chrome/browser/ui/webui/extensions/extension_loader_handler.cc:252: NotifyFrontendOfFailure(failure->file_path, I'd still like to be able to notify the frontend in one big batch, and have it sort it out. Doing it this way could result in a flicker. https://codereview.chromium.org/342003005/diff/180001/chrome/browser/ui/webui... File chrome/browser/ui/webui/extensions/extension_loader_handler.h (right): https://codereview.chromium.org/342003005/diff/180001/chrome/browser/ui/webui... chrome/browser/ui/webui/extensions/extension_loader_handler.h:98: bool display_ready_; nit: even though it's pretty obvious, put a comment (specifically, *when* is the display considered ready).
Still working on the reloading signal, ReloadExtension noisy boolean, and sending multiple load failures at once to the JS. I addressed the smaller issues, though. https://codereview.chromium.org/342003005/diff/180001/chrome/app/generated_re... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/342003005/diff/180001/chrome/app/generated_re... chrome/app/generated_resources.grd:4765: + <message name="IDS_EXTENSIONS_LOAD_ADDITIONAL_FAILURES" desc="The text displayed above additional failures."> On 2014/07/02 17:46:50, Devlin wrote: > "...additional extension load failures" Done. https://codereview.chromium.org/342003005/diff/180001/chrome/browser/backgrou... File chrome/browser/background/background_contents_service.cc (right): https://codereview.chromium.org/342003005/diff/180001/chrome/browser/backgrou... chrome/browser/background/background_contents_service.cc:132: ReloadExtension(copied_extension_id, true); On 2014/07/02 17:46:50, Devlin wrote: > all of these should have anonymous bool comments... > > On second thought.... I'm beginning to doubt whether or not we should force > every caller to do this. It creates a bit more noise in the CL than I was > expecting. Let's go back to making two functions, > ReloadExtension() and ReloadExtensionWithQuietFailure(). > Sorry for the extra work - I should have looked at all the call sites before I > requested it :/ Not a problem! This is why I suggested overloading the method. The only problem here is that ReloadExtension needs to have the boolean argument, since it creates and uses the unpacked installer. The other option would be having a flag in the class that's used in that method, and another method to set that flag. That seems messy, but I couldn't simply add a ReloadExtensionWithQuietFailure without either copying the method with a minor change, or adding the boolean flag. That's why I suggested having one that takes the boolean flag, and overloading it with one that doesn't that can call the other with true by default. I'll take a look at it and see if I can come up with any other solutions. https://codereview.chromium.org/342003005/diff/180001/chrome/browser/resource... File chrome/browser/resources/extensions/extension_loader.js (right): https://codereview.chromium.org/342003005/diff/180001/chrome/browser/resource... chrome/browser/resources/extensions/extension_loader.js:31: this.path_ = filePath; On 2014/07/02 17:46:50, Devlin wrote: > no trailing _ if it's not a private var. Done. https://codereview.chromium.org/342003005/diff/180001/chrome/browser/resource... chrome/browser/resources/extensions/extension_loader.js:74: * @type {Array} On 2014/07/02 17:46:50, Devlin wrote: > There's a syntax for saying what the array is full of, I think. Check on the > closure compiler docs or look around the source code more. Done. https://codereview.chromium.org/342003005/diff/180001/chrome/browser/resource... chrome/browser/resources/extensions/extension_loader.js:103: this.additional_.hidden = false; On 2014/07/02 17:46:50, Devlin wrote: > Let's put the logic for showing/hidding |additional| in show(). Done. https://codereview.chromium.org/342003005/diff/180001/chrome/browser/ui/webui... File chrome/browser/ui/webui/extensions/extension_loader_handler.cc (right): https://codereview.chromium.org/342003005/diff/180001/chrome/browser/ui/webui... chrome/browser/ui/webui/extensions/extension_loader_handler.cc:134: FailureData(base::FilePath file_path, On 2014/07/02 17:46:50, Devlin wrote: > this should take const file path and string. Done. https://codereview.chromium.org/342003005/diff/180001/chrome/browser/ui/webui... chrome/browser/ui/webui/extensions/extension_loader_handler.cc:152: ExtensionLoaderHandler::FailureData::FailureData(base::FilePath file_path, On 2014/07/02 17:46:50, Devlin wrote: > prefer this one :) Done. https://codereview.chromium.org/342003005/diff/180001/chrome/browser/ui/webui... chrome/browser/ui/webui/extensions/extension_loader_handler.cc:208: base::Bind(&ExtensionLoaderHandler::HandleGiveUp, On 2014/07/02 17:46:50, Devlin wrote: > Even though we call it give up in the text, let's change this to what it > actually is - ignore failure. Done. https://codereview.chromium.org/342003005/diff/180001/chrome/browser/ui/webui... File chrome/browser/ui/webui/extensions/extension_loader_handler.h (right): https://codereview.chromium.org/342003005/diff/180001/chrome/browser/ui/webui... chrome/browser/ui/webui/extensions/extension_loader_handler.h:98: bool display_ready_; On 2014/07/02 17:46:50, Devlin wrote: > nit: even though it's pretty obvious, put a comment (specifically, *when* is the > display considered ready). Done.
https://codereview.chromium.org/342003005/diff/180001/chrome/browser/backgrou... File chrome/browser/background/background_contents_service.cc (right): https://codereview.chromium.org/342003005/diff/180001/chrome/browser/backgrou... chrome/browser/background/background_contents_service.cc:132: ReloadExtension(copied_extension_id, true); On 2014/07/02 19:13:54, gpdavis wrote: > On 2014/07/02 17:46:50, Devlin wrote: > > all of these should have anonymous bool comments... > > > > On second thought.... I'm beginning to doubt whether or not we should force > > every caller to do this. It creates a bit more noise in the CL than I was > > expecting. Let's go back to making two functions, > > ReloadExtension() and ReloadExtensionWithQuietFailure(). > > Sorry for the extra work - I should have looked at all the call sites before I > > requested it :/ > > Not a problem! This is why I suggested overloading the method. > > The only problem here is that ReloadExtension needs to have the boolean > argument, since it creates and uses the unpacked installer. The other option > would be having a flag in the class that's used in that method, and another > method to set that flag. That seems messy, but I couldn't simply add a > ReloadExtensionWithQuietFailure without either copying the method with a minor > change, or adding the boolean flag. That's why I suggested having one that > takes the boolean flag, and overloading it with one that doesn't that can call > the other with true by default. > > I'll take a look at it and see if I can come up with any other solutions. Our usual tactic for this is something like: ExtensionService { public: ReloadExtension() { ReloadExtensionImpl(true /* be noisy */); } ReloadExtensionWithQuietFailure() { ReloadExtensionImpl(false /* don't be noisy */); } private: ReloadExtensionImpl(bool be_noisy) { ... } } It keeps the public interface fairly clean, and makes it clear what's happening in all cases.
I went ahead and implemented passing a ListValue with all the failure information to avoid multiple javascript function calls. I have high hopes for this implementation ;) The FailureData struct was no longer necessary, so the code was greatly simplified. I also took care of the ReloadExtensionImpl bit as well. Eager to get some feedback on this one!
Getting closer! :) https://codereview.chromium.org/342003005/diff/230001/chrome/browser/extensio... File chrome/browser/extensions/extension_service.h (right): https://codereview.chromium.org/342003005/diff/230001/chrome/browser/extensio... chrome/browser/extensions/extension_service.h:240: // Public method for ReloadExtensionImpl. Allows noisy failures. No point in saying "Public method for x". Just say "Allows noisy failure." https://codereview.chromium.org/342003005/diff/230001/chrome/browser/extensio... chrome/browser/extensions/extension_service.h:242: ReloadExtensionImpl(extension_id, true /* be_noisy */); These calls should actually be in the .cc file. https://codereview.chromium.org/342003005/diff/230001/chrome/browser/resource... File chrome/browser/resources/extensions/extension_load_error.html (right): https://codereview.chromium.org/342003005/diff/230001/chrome/browser/resource... chrome/browser/resources/extensions/extension_load_error.html:21: <div> Why is the div visible at all, if the only child is hidden? Actually, why do we need the div at all? https://codereview.chromium.org/342003005/diff/230001/chrome/browser/resource... File chrome/browser/resources/extensions/extension_loader.js (right): https://codereview.chromium.org/342003005/diff/230001/chrome/browser/resource... chrome/browser/resources/extensions/extension_loader.js:5: window.addEventListener('beforeunload', function() { Didn't we have a way of potentially getting rid of this? https://codereview.chromium.org/342003005/diff/230001/chrome/browser/resource... chrome/browser/resources/extensions/extension_loader.js:101: add: function(path, reason, manifest) { Can this be private? https://codereview.chromium.org/342003005/diff/230001/chrome/browser/resource... chrome/browser/resources/extensions/extension_loader.js:107: * Remove a failure from failures_ array. If this was the last failure, nit: |failures_| nit: 1 space after period. https://codereview.chromium.org/342003005/diff/230001/chrome/browser/resource... chrome/browser/resources/extensions/extension_loader.js:114: if (this.failures_.length == 0) { nit: no brackets for one-line ifs. https://codereview.chromium.org/342003005/diff/230001/chrome/browser/resource... chrome/browser/resources/extensions/extension_loader.js:128: var failure = this.failures_[this.failures_.length - 1]; We should assert that there is at least one failure. https://codereview.chromium.org/342003005/diff/230001/chrome/browser/resource... chrome/browser/resources/extensions/extension_loader.js:141: } else if (this.failures_.length > 1) { We can know that there is >= 1 failure, so a simple "else" is enough. https://codereview.chromium.org/342003005/diff/230001/chrome/browser/resource... chrome/browser/resources/extensions/extension_loader.js:143: this.additional_.textContent = I think it would be cleaner to have this be a list element. Helps with the ARIA, among other things. https://codereview.chromium.org/342003005/diff/230001/chrome/browser/resource... chrome/browser/resources/extensions/extension_loader.js:200: ExtensionLoader.getInstance().notifyFailed(failures[i].path, We should add these in batches, because otherwise you could potentially see a rapid flicker as they are all added (and it's less efficient). https://codereview.chromium.org/342003005/diff/230001/chrome/browser/ui/webui... File chrome/browser/ui/webui/extensions/extension_loader_handler.cc (right): https://codereview.chromium.org/342003005/diff/230001/chrome/browser/ui/webui... chrome/browser/ui/webui/extensions/extension_loader_handler.cc:195: LoadUnpackedExtensionImpl(failed_paths_.back()); We should probably pop, then run. https://codereview.chromium.org/342003005/diff/230001/chrome/browser/ui/webui... chrome/browser/ui/webui/extensions/extension_loader_handler.cc:265: base::StringValue* file_value = inline these. https://codereview.chromium.org/342003005/diff/230001/chrome/browser/ui/webui... chrome/browser/ui/webui/extensions/extension_loader_handler.cc:276: base::DictionaryValue* failure = new base::DictionaryValue(); For better or worse, we're overly paranoid about memory leaks. Please put this in a scoped ptr. https://codereview.chromium.org/342003005/diff/230001/chrome/browser/ui/webui... chrome/browser/ui/webui/extensions/extension_loader_handler.cc:283: if (display_ready_) { nit: no brackets around single-line ifs. https://codereview.chromium.org/342003005/diff/230001/chrome/browser/ui/webui... File chrome/browser/ui/webui/extensions/extension_loader_handler.h (right): https://codereview.chromium.org/342003005/diff/230001/chrome/browser/ui/webui... chrome/browser/ui/webui/extensions/extension_loader_handler.h:15: #include "base/memory/scoped_vector.h" Don't need this, right?
https://codereview.chromium.org/342003005/diff/230001/chrome/browser/extensio... File chrome/browser/extensions/extension_service.h (right): https://codereview.chromium.org/342003005/diff/230001/chrome/browser/extensio... chrome/browser/extensions/extension_service.h:240: // Public method for ReloadExtensionImpl. Allows noisy failures. On 2014/07/07 20:44:21, Devlin wrote: > No point in saying "Public method for x". Just say "Allows noisy failure." Done. https://codereview.chromium.org/342003005/diff/230001/chrome/browser/extensio... chrome/browser/extensions/extension_service.h:242: ReloadExtensionImpl(extension_id, true /* be_noisy */); On 2014/07/07 20:44:21, Devlin wrote: > These calls should actually be in the .cc file. Done. https://codereview.chromium.org/342003005/diff/230001/chrome/browser/resource... File chrome/browser/resources/extensions/extension_load_error.html (right): https://codereview.chromium.org/342003005/diff/230001/chrome/browser/resource... chrome/browser/resources/extensions/extension_load_error.html:21: <div> On 2014/07/07 20:44:21, Devlin wrote: > Why is the div visible at all, if the only child is hidden? Actually, why do we > need the div at all? Replaced with list. Done. https://codereview.chromium.org/342003005/diff/230001/chrome/browser/resource... File chrome/browser/resources/extensions/extension_loader.js (right): https://codereview.chromium.org/342003005/diff/230001/chrome/browser/resource... chrome/browser/resources/extensions/extension_loader.js:5: window.addEventListener('beforeunload', function() { On 2014/07/07 20:44:21, Devlin wrote: > Didn't we have a way of potentially getting rid of this? I gave it a shot, and it didn't seem to work properly. I don't remember exactly what it was that went wrong. This seems like the most direct and least messy way to go about doing this. Is this something that really should not be here? I can try and replace it with something else, but if I recall correctly, I couldn't find a place for it that would work. https://codereview.chromium.org/342003005/diff/230001/chrome/browser/resource... chrome/browser/resources/extensions/extension_loader.js:101: add: function(path, reason, manifest) { On 2014/07/07 20:44:21, Devlin wrote: > Can this be private? Done. https://codereview.chromium.org/342003005/diff/230001/chrome/browser/resource... chrome/browser/resources/extensions/extension_loader.js:107: * Remove a failure from failures_ array. If this was the last failure, On 2014/07/07 20:44:21, Devlin wrote: > nit: |failures_| > nit: 1 space after period. Done. https://codereview.chromium.org/342003005/diff/230001/chrome/browser/resource... chrome/browser/resources/extensions/extension_loader.js:114: if (this.failures_.length == 0) { On 2014/07/07 20:44:21, Devlin wrote: > nit: no brackets for one-line ifs. Done. https://codereview.chromium.org/342003005/diff/230001/chrome/browser/resource... chrome/browser/resources/extensions/extension_loader.js:128: var failure = this.failures_[this.failures_.length - 1]; On 2014/07/07 20:44:21, Devlin wrote: > We should assert that there is at least one failure. Done. https://codereview.chromium.org/342003005/diff/230001/chrome/browser/resource... chrome/browser/resources/extensions/extension_loader.js:141: } else if (this.failures_.length > 1) { On 2014/07/07 20:44:21, Devlin wrote: > We can know that there is >= 1 failure, so a simple "else" is enough. Done. https://codereview.chromium.org/342003005/diff/230001/chrome/browser/resource... chrome/browser/resources/extensions/extension_loader.js:143: this.additional_.textContent = On 2014/07/07 20:44:21, Devlin wrote: > I think it would be cleaner to have this be a list element. Helps with the > ARIA, among other things. Done. https://codereview.chromium.org/342003005/diff/230001/chrome/browser/resource... chrome/browser/resources/extensions/extension_loader.js:200: ExtensionLoader.getInstance().notifyFailed(failures[i].path, On 2014/07/07 20:44:21, Devlin wrote: > We should add these in batches, because otherwise you could potentially see a > rapid flicker as they are all added (and it's less efficient). Done. https://codereview.chromium.org/342003005/diff/230001/chrome/browser/ui/webui... File chrome/browser/ui/webui/extensions/extension_loader_handler.cc (right): https://codereview.chromium.org/342003005/diff/230001/chrome/browser/ui/webui... chrome/browser/ui/webui/extensions/extension_loader_handler.cc:195: LoadUnpackedExtensionImpl(failed_paths_.back()); On 2014/07/07 20:44:22, Devlin wrote: > We should probably pop, then run. But pop_back deletes the element and doesn't return it. I decided to pop_back afterward because there are two possible cases: the reload is successful, in which the path is popped off, or the reload fails, and the path is pushed on and then popped back off. Either way, the last element will be correct. But I understand why we might want to change it anyway. I can just create a filepath from the last element in the vector, then pop_back, and pass the filepath onward. https://codereview.chromium.org/342003005/diff/230001/chrome/browser/ui/webui... chrome/browser/ui/webui/extensions/extension_loader_handler.cc:265: base::StringValue* file_value = On 2014/07/07 20:44:21, Devlin wrote: > inline these. Done. https://codereview.chromium.org/342003005/diff/230001/chrome/browser/ui/webui... chrome/browser/ui/webui/extensions/extension_loader_handler.cc:276: base::DictionaryValue* failure = new base::DictionaryValue(); On 2014/07/07 20:44:21, Devlin wrote: > For better or worse, we're overly paranoid about memory leaks. Please put this > in a scoped ptr. You can never be overly paranoid about memory leaks ;) I used a regular pointer because pop_back should destruct the element. I tried running it with a scoped pointer, and it crashed. base::ListValue::Clear() actually deletes all the items in the list, so there shouldn't be a memory issue here. Correct me if I'm misunderstanding, though. https://code.google.com/p/chromium/codesearch#chromium/src/base/values.cc&rcl... https://codereview.chromium.org/342003005/diff/230001/chrome/browser/ui/webui... chrome/browser/ui/webui/extensions/extension_loader_handler.cc:283: if (display_ready_) { On 2014/07/07 20:44:21, Devlin wrote: > nit: no brackets around single-line ifs. Done. https://codereview.chromium.org/342003005/diff/230001/chrome/browser/ui/webui... File chrome/browser/ui/webui/extensions/extension_loader_handler.h (right): https://codereview.chromium.org/342003005/diff/230001/chrome/browser/ui/webui... chrome/browser/ui/webui/extensions/extension_loader_handler.h:15: #include "base/memory/scoped_vector.h" On 2014/07/07 20:44:22, Devlin wrote: > Don't need this, right? Done.
https://codereview.chromium.org/342003005/diff/230001/chrome/browser/resource... File chrome/browser/resources/extensions/extension_loader.js (right): https://codereview.chromium.org/342003005/diff/230001/chrome/browser/resource... chrome/browser/resources/extensions/extension_loader.js:5: window.addEventListener('beforeunload', function() { On 2014/07/09 01:35:57, gpdavis wrote: > On 2014/07/07 20:44:21, Devlin wrote: > > Didn't we have a way of potentially getting rid of this? > > I gave it a shot, and it didn't seem to work properly. I don't remember exactly > what it was that went wrong. This seems like the most direct and least messy > way to go about doing this. Is this something that really should not be here? > I can try and replace it with something else, but if I recall correctly, I > couldn't find a place for it that would work. I highly doubt that this is the best solution. The settings page is incredibly complex. Which window does |window| refer to? What if the frame is destroyed, but not the window? What happens to extensions? Or the modifications to the now-defunct DOM? Can you find out what it was that didn't work with the other approach? https://codereview.chromium.org/342003005/diff/230001/chrome/browser/ui/webui... File chrome/browser/ui/webui/extensions/extension_loader_handler.cc (right): https://codereview.chromium.org/342003005/diff/230001/chrome/browser/ui/webui... chrome/browser/ui/webui/extensions/extension_loader_handler.cc:195: LoadUnpackedExtensionImpl(failed_paths_.back()); On 2014/07/09 01:35:57, gpdavis wrote: > On 2014/07/07 20:44:22, Devlin wrote: > > We should probably pop, then run. > > But pop_back deletes the element and doesn't return it. I decided to pop_back > afterward because there are two possible cases: the reload is successful, in > which the path is popped off, or the reload fails, and the path is pushed on and > then popped back off. Either way, the last element will be correct. > > But I understand why we might want to change it anyway. I can just create a > filepath from the last element in the vector, then pop_back, and pass the > filepath onward. Right, so because pop_back doesn't return it (one of the worst choices in the STL, I swear...), you need to get a copy of it before. base::FilePath failed_path = failed_paths_.back(); failed_paths_.pop_back(); LoadUnpackedExtensionImpl(failed_path); https://codereview.chromium.org/342003005/diff/230001/chrome/browser/ui/webui... chrome/browser/ui/webui/extensions/extension_loader_handler.cc:276: base::DictionaryValue* failure = new base::DictionaryValue(); On 2014/07/09 01:35:57, gpdavis wrote: > On 2014/07/07 20:44:21, Devlin wrote: > > For better or worse, we're overly paranoid about memory leaks. Please put > this > > in a scoped ptr. > > You can never be overly paranoid about memory leaks ;) I used a regular pointer > because pop_back should destruct the element. I tried running it with a scoped > pointer, and it crashed. > > base::ListValue::Clear() actually deletes all the items in the list, so there > shouldn't be a memory issue here. Correct me if I'm misunderstanding, though. > > https://code.google.com/p/chromium/codesearch#chromium/src/base/values.cc&rcl... Right, but we like extremely clear ownership models. That is, there should never (if we can help it) have an unowned object, and, at line 276 (in this patch), it is unowned. Instead, it should be: scoped_ptr<base::DictionaryValue> failure(new base::DictionaryValue()); failure->Set(...); failures_.Append(failure.release()); Then the object is always owned, and will always be deleted, even if we were to say insert an early return somewhere. A completely contrived example: We realize that we are sometimes showing failures with non-existent paths. We fix it by changing this to: base::DictionaryValue* failure = new base::DictionaryValue(); failure->Set("error", error); failure->Set("manifest", manifest); // We obviously don't want to show a path that doesn't exist. if (!base::PathExists(base::FilePath(file_path))) return; failure->SetPath("path", file_path); failures_.Append(failure); But wait! |failure| leaks!! Would this ever happen? Perhaps not. But, like, I said, we are overly paranoid. :) https://codereview.chromium.org/342003005/diff/270001/chrome/browser/extensio... File chrome/browser/extensions/extension_service.cc (right): https://codereview.chromium.org/342003005/diff/270001/chrome/browser/extensio... chrome/browser/extensions/extension_service.cc:233: void ExtensionService::ReloadExtension(const std::string& extension_id) { In theory, methods should be defined in the .cc file in the same order they are declared in the .h file. Admittedly, extension_service does not do this. But no reason to make it worse here. https://codereview.chromium.org/342003005/diff/270001/chrome/browser/extensio... chrome/browser/extensions/extension_service.cc:608: void ExtensionService::ReloadExtensionImpl( We can make an exception for the method definition order for this so that we preserve git blame history. https://codereview.chromium.org/342003005/diff/270001/chrome/browser/resource... File chrome/browser/resources/extensions/extension_load_error.html (right): https://codereview.chromium.org/342003005/diff/270001/chrome/browser/resource... chrome/browser/resources/extensions/extension_load_error.html:22: <span>Additional failures:</span> i18n is still important... https://codereview.chromium.org/342003005/diff/270001/chrome/browser/resource... File chrome/browser/resources/extensions/extension_loader.js (right): https://codereview.chromium.org/342003005/diff/270001/chrome/browser/resource... chrome/browser/resources/extensions/extension_loader.js:101: * @param {string} path The path from which the extension was loaded. Update comment. https://codereview.chromium.org/342003005/diff/270001/chrome/browser/resource... chrome/browser/resources/extensions/extension_loader.js:108: if (this.failures_.length > 0) { no brackets around single-line ifs. https://codereview.chromium.org/342003005/diff/270001/chrome/browser/resource... chrome/browser/resources/extensions/extension_loader.js:109: this.failures_[this.failures_.length - 1].listElement.hidden = false; |listElement| should be declared in the constructor. https://codereview.chromium.org/342003005/diff/270001/chrome/browser/resource... chrome/browser/resources/extensions/extension_loader.js:111: var listItem; initialize. https://codereview.chromium.org/342003005/diff/270001/chrome/browser/resource... chrome/browser/resources/extensions/extension_loader.js:115: listItem.innerHTML = failures[i].path; innerHTML is the devil. Never use it if you can help it. https://codereview.chromium.org/342003005/diff/270001/chrome/browser/resource... chrome/browser/resources/extensions/extension_loader.js:116: this.additional_.appendChild(listItem); I don't see where these are ever removed? https://codereview.chromium.org/342003005/diff/270001/chrome/browser/ui/webui... File chrome/browser/ui/webui/extensions/extension_loader_handler.h (right): https://codereview.chromium.org/342003005/diff/270001/chrome/browser/ui/webui... chrome/browser/ui/webui/extensions/extension_loader_handler.h:22: class ListValue; No need to forward-declare this now.
https://codereview.chromium.org/342003005/diff/230001/chrome/browser/resource... File chrome/browser/resources/extensions/extension_loader.js (right): https://codereview.chromium.org/342003005/diff/230001/chrome/browser/resource... chrome/browser/resources/extensions/extension_loader.js:5: window.addEventListener('beforeunload', function() { On 2014/07/09 19:53:17, Devlin wrote: > On 2014/07/09 01:35:57, gpdavis wrote: > > On 2014/07/07 20:44:21, Devlin wrote: > > > Didn't we have a way of potentially getting rid of this? > > > > I gave it a shot, and it didn't seem to work properly. I don't remember > exactly > > what it was that went wrong. This seems like the most direct and least messy > > way to go about doing this. Is this something that really should not be here? > > > I can try and replace it with something else, but if I recall correctly, I > > couldn't find a place for it that would work. > > I highly doubt that this is the best solution. The settings page is incredibly > complex. Which window does |window| refer to? What if the frame is destroyed, > but not the window? What happens to extensions? Or the modifications to the > now-defunct DOM? > > Can you find out what it was that didn't work with the other approach? Well, I figured out what I attempted before, but this time it worked. Must have just made a mistake last time and wasn't sure what to do when it blew up. Problem solved now: I made the loader handler into a WebContentsObserver and had it override the same function that the settings handler uses to trigger the refresh. Works just fine. https://codereview.chromium.org/342003005/diff/230001/chrome/browser/ui/webui... File chrome/browser/ui/webui/extensions/extension_loader_handler.cc (right): https://codereview.chromium.org/342003005/diff/230001/chrome/browser/ui/webui... chrome/browser/ui/webui/extensions/extension_loader_handler.cc:195: LoadUnpackedExtensionImpl(failed_paths_.back()); On 2014/07/09 19:53:17, Devlin wrote: > On 2014/07/09 01:35:57, gpdavis wrote: > > On 2014/07/07 20:44:22, Devlin wrote: > > > We should probably pop, then run. > > > > But pop_back deletes the element and doesn't return it. I decided to pop_back > > afterward because there are two possible cases: the reload is successful, in > > which the path is popped off, or the reload fails, and the path is pushed on > and > > then popped back off. Either way, the last element will be correct. > > > > But I understand why we might want to change it anyway. I can just create a > > filepath from the last element in the vector, then pop_back, and pass the > > filepath onward. > > Right, so because pop_back doesn't return it (one of the worst choices in the > STL, I swear...), you need to get a copy of it before. > > base::FilePath failed_path = failed_paths_.back(); > failed_paths_.pop_back(); > LoadUnpackedExtensionImpl(failed_path); Done. https://codereview.chromium.org/342003005/diff/230001/chrome/browser/ui/webui... chrome/browser/ui/webui/extensions/extension_loader_handler.cc:276: base::DictionaryValue* failure = new base::DictionaryValue(); On 2014/07/09 19:53:17, Devlin wrote: > On 2014/07/09 01:35:57, gpdavis wrote: > > On 2014/07/07 20:44:21, Devlin wrote: > > > For better or worse, we're overly paranoid about memory leaks. Please put > > this > > > in a scoped ptr. > > > > You can never be overly paranoid about memory leaks ;) I used a regular > pointer > > because pop_back should destruct the element. I tried running it with a > scoped > > pointer, and it crashed. > > > > base::ListValue::Clear() actually deletes all the items in the list, so there > > shouldn't be a memory issue here. Correct me if I'm misunderstanding, though. > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/base/values.cc&rcl... > > Right, but we like extremely clear ownership models. That is, there should > never (if we can help it) have an unowned object, and, at line 276 (in this > patch), it is unowned. Instead, it should be: > > scoped_ptr<base::DictionaryValue> failure(new base::DictionaryValue()); > failure->Set(...); > failures_.Append(failure.release()); > > Then the object is always owned, and will always be deleted, even if we were to > say insert an early return somewhere. > > A completely contrived example: > We realize that we are sometimes showing failures with non-existent paths. We > fix it by changing this to: > > base::DictionaryValue* failure = new base::DictionaryValue(); > failure->Set("error", error); > failure->Set("manifest", manifest); > // We obviously don't want to show a path that doesn't exist. > if (!base::PathExists(base::FilePath(file_path))) > return; > failure->SetPath("path", file_path); > failures_.Append(failure); > > But wait! |failure| leaks!! > > Would this ever happen? Perhaps not. But, like, I said, we are overly > paranoid. :) Ah, gotcha. I must have been doing something wrong before because it crashed with a rather cryptic stacktrace the last time I did it. I think I screwed up the initialization of the scoped pointer, and it snuck past the compiler somehow. Anyhow, taken care of. I also scoped manifest_value since it has the same vulnerability as it stands in this patch set. https://codereview.chromium.org/342003005/diff/270001/chrome/browser/extensio... File chrome/browser/extensions/extension_service.cc (right): https://codereview.chromium.org/342003005/diff/270001/chrome/browser/extensio... chrome/browser/extensions/extension_service.cc:233: void ExtensionService::ReloadExtension(const std::string& extension_id) { On 2014/07/09 19:53:17, Devlin wrote: > In theory, methods should be defined in the .cc file in the same order they are > declared in the .h file. Admittedly, extension_service does not do this. But > no reason to make it worse here. Done. https://codereview.chromium.org/342003005/diff/270001/chrome/browser/extensio... chrome/browser/extensions/extension_service.cc:608: void ExtensionService::ReloadExtensionImpl( On 2014/07/09 19:53:17, Devlin wrote: > We can make an exception for the method definition order for this so that we > preserve git blame history. Done. https://codereview.chromium.org/342003005/diff/270001/chrome/browser/resource... File chrome/browser/resources/extensions/extension_load_error.html (right): https://codereview.chromium.org/342003005/diff/270001/chrome/browser/resource... chrome/browser/resources/extensions/extension_load_error.html:22: <span>Additional failures:</span> On 2014/07/09 19:53:17, Devlin wrote: > i18n is still important... Oops! Mistakes were made. Taken care of. https://codereview.chromium.org/342003005/diff/270001/chrome/browser/resource... File chrome/browser/resources/extensions/extension_loader.js (right): https://codereview.chromium.org/342003005/diff/270001/chrome/browser/resource... chrome/browser/resources/extensions/extension_loader.js:101: * @param {string} path The path from which the extension was loaded. On 2014/07/09 19:53:17, Devlin wrote: > Update comment. Done. https://codereview.chromium.org/342003005/diff/270001/chrome/browser/resource... chrome/browser/resources/extensions/extension_loader.js:108: if (this.failures_.length > 0) { On 2014/07/09 19:53:17, Devlin wrote: > no brackets around single-line ifs. Done. https://codereview.chromium.org/342003005/diff/270001/chrome/browser/resource... chrome/browser/resources/extensions/extension_loader.js:109: this.failures_[this.failures_.length - 1].listElement.hidden = false; On 2014/07/09 19:53:17, Devlin wrote: > |listElement| should be declared in the constructor. Done. https://codereview.chromium.org/342003005/diff/270001/chrome/browser/resource... chrome/browser/resources/extensions/extension_loader.js:111: var listItem; On 2014/07/09 19:53:17, Devlin wrote: > initialize. Done. https://codereview.chromium.org/342003005/diff/270001/chrome/browser/resource... chrome/browser/resources/extensions/extension_loader.js:115: listItem.innerHTML = failures[i].path; On 2014/07/09 19:53:17, Devlin wrote: > innerHTML is the devil. Never use it if you can help it. Done. https://codereview.chromium.org/342003005/diff/270001/chrome/browser/resource... chrome/browser/resources/extensions/extension_loader.js:116: this.additional_.appendChild(listItem); On 2014/07/09 19:53:17, Devlin wrote: > I don't see where these are ever removed? Whoops. That's a pretty significant thing to forget. Done. https://codereview.chromium.org/342003005/diff/270001/chrome/browser/ui/webui... File chrome/browser/ui/webui/extensions/extension_loader_handler.h (right): https://codereview.chromium.org/342003005/diff/270001/chrome/browser/ui/webui... chrome/browser/ui/webui/extensions/extension_loader_handler.h:22: class ListValue; On 2014/07/09 19:53:17, Devlin wrote: > No need to forward-declare this now. Done.
Screenshot for this one too, please. :) https://codereview.chromium.org/342003005/diff/290001/chrome/browser/resource... File chrome/browser/resources/extensions/extension_load_error.html (right): https://codereview.chromium.org/342003005/diff/290001/chrome/browser/resource... chrome/browser/resources/extensions/extension_load_error.html:22: <span i18n-content="extensionLoadAdditionalFailures"></span> I'll let someone more familiar with this confirm or deny, but I think maybe this would be better as: <div id="extension-load-error-additional" hidden> <span ...></span> <ul></ul> </div> There may be interesting accessibility occurrences with non-list items in a list. https://codereview.chromium.org/342003005/diff/290001/chrome/browser/resource... File chrome/browser/resources/extensions/extension_loader.js (right): https://codereview.chromium.org/342003005/diff/290001/chrome/browser/resource... chrome/browser/resources/extensions/extension_loader.js:28: * @param {Element} listElement The HTML element used for displaying the HTMLLIElement https://codereview.chromium.org/342003005/diff/290001/chrome/browser/resource... chrome/browser/resources/extensions/extension_loader.js:70: * @type {HTMLPreElement} Update. https://codereview.chromium.org/342003005/diff/290001/chrome/browser/resource... chrome/browser/resources/extensions/extension_loader.js:116: failures[failures.length - 1].listElement.hidden = true; I'd prefer you use |failures_| for this (even though they will be the same, it makes it clearer, as you want to mark it for the last element of failures_, which just happens to also be the last element of failures). https://codereview.chromium.org/342003005/diff/290001/chrome/browser/resource... chrome/browser/resources/extensions/extension_loader.js:130: if (this.failures_.length > 0) Why three ifs? if (this.failures_.length > 0) { this.failures_[this.failures_.length - 1].listElement.hidden = true; this.show_(); } else { this.hidden = true; } https://codereview.chromium.org/342003005/diff/290001/chrome/browser/resource... chrome/browser/resources/extensions/extension_loader.js:157: if (this.failures_.length == 1) this.additional_.hidden = this.failures_.length == 1; https://codereview.chromium.org/342003005/diff/290001/chrome/browser/resource... chrome/browser/resources/extensions/extension_loader.js:193: * errors, manifests, and listElements. It doesn't contain listElements, though, right? (Same for the other places you mention this.) https://codereview.chromium.org/342003005/diff/290001/chrome/browser/ui/webui... File chrome/browser/ui/webui/extensions/extension_loader_handler.cc (right): https://codereview.chromium.org/342003005/diff/290001/chrome/browser/ui/webui... chrome/browser/ui/webui/extensions/extension_loader_handler.cc:188: if (reload_type != content::NavigationController::NO_RELOAD) { no brackets around single-line ifs. https://codereview.chromium.org/342003005/diff/290001/chrome/browser/ui/webui... chrome/browser/ui/webui/extensions/extension_loader_handler.cc:200: const base::FilePath file_path(failed_paths_.back()); nit: I find this cleaner as file_path = failed_paths.back(); https://codereview.chromium.org/342003005/diff/290001/chrome/browser/ui/webui... chrome/browser/ui/webui/extensions/extension_loader_handler.cc:233: failed_paths_.push_back(file_path); Let's reduce the risk of a race a little bit by moving this to AddFailure(). https://codereview.chromium.org/342003005/diff/290001/chrome/browser/ui/webui... File chrome/browser/ui/webui/extensions/extension_loader_handler.h (right): https://codereview.chromium.org/342003005/diff/290001/chrome/browser/ui/webui... chrome/browser/ui/webui/extensions/extension_loader_handler.h:52: const GURL& url, nit: put the "implementations" next to each other in the header (and the cc).
Screenshot inbound: http://i.imgur.com/Qepy0Rg.png https://codereview.chromium.org/342003005/diff/290001/chrome/browser/resource... File chrome/browser/resources/extensions/extension_loader.js (right): https://codereview.chromium.org/342003005/diff/290001/chrome/browser/resource... chrome/browser/resources/extensions/extension_loader.js:28: * @param {Element} listElement The HTML element used for displaying the On 2014/07/09 23:03:32, Devlin wrote: > HTMLLIElement Done. https://codereview.chromium.org/342003005/diff/290001/chrome/browser/resource... chrome/browser/resources/extensions/extension_loader.js:70: * @type {HTMLPreElement} On 2014/07/09 23:03:32, Devlin wrote: > Update. Done. https://codereview.chromium.org/342003005/diff/290001/chrome/browser/resource... chrome/browser/resources/extensions/extension_loader.js:116: failures[failures.length - 1].listElement.hidden = true; On 2014/07/09 23:03:32, Devlin wrote: > I'd prefer you use |failures_| for this (even though they will be the same, it > makes it clearer, as you want to mark it for the last element of failures_, > which just happens to also be the last element of failures). Fair enough. I chose to use failures just because it was less cluttered because it doesn't need this. https://codereview.chromium.org/342003005/diff/290001/chrome/browser/resource... chrome/browser/resources/extensions/extension_loader.js:130: if (this.failures_.length > 0) On 2014/07/09 23:03:32, Devlin wrote: > Why three ifs? > if (this.failures_.length > 0) { > this.failures_[this.failures_.length - 1].listElement.hidden = true; > this.show_(); > } else { > this.hidden = true; > } Good point... this is the result of adding conditionals in the middle of a train of thought. Messy logic. Fixed. https://codereview.chromium.org/342003005/diff/290001/chrome/browser/resource... chrome/browser/resources/extensions/extension_loader.js:157: if (this.failures_.length == 1) On 2014/07/09 23:03:32, Devlin wrote: > this.additional_.hidden = this.failures_.length == 1; Nice. https://codereview.chromium.org/342003005/diff/290001/chrome/browser/resource... chrome/browser/resources/extensions/extension_loader.js:193: * errors, manifests, and listElements. On 2014/07/09 23:03:32, Devlin wrote: > It doesn't contain listElements, though, right? (Same for the other places you > mention this.) Oops. You're right; these objects aren't yet Failures. https://codereview.chromium.org/342003005/diff/290001/chrome/browser/ui/webui... File chrome/browser/ui/webui/extensions/extension_loader_handler.cc (right): https://codereview.chromium.org/342003005/diff/290001/chrome/browser/ui/webui... chrome/browser/ui/webui/extensions/extension_loader_handler.cc:188: if (reload_type != content::NavigationController::NO_RELOAD) { On 2014/07/09 23:03:32, Devlin wrote: > no brackets around single-line ifs. Dang, I keep forgetting to remove those after I take out debug statements. Done. https://codereview.chromium.org/342003005/diff/290001/chrome/browser/ui/webui... chrome/browser/ui/webui/extensions/extension_loader_handler.cc:200: const base::FilePath file_path(failed_paths_.back()); On 2014/07/09 23:03:32, Devlin wrote: > nit: I find this cleaner as file_path = failed_paths.back(); Done. https://codereview.chromium.org/342003005/diff/290001/chrome/browser/ui/webui... chrome/browser/ui/webui/extensions/extension_loader_handler.cc:233: failed_paths_.push_back(file_path); On 2014/07/09 23:03:32, Devlin wrote: > Let's reduce the risk of a race a little bit by moving this to AddFailure(). Done. https://codereview.chromium.org/342003005/diff/290001/chrome/browser/ui/webui... File chrome/browser/ui/webui/extensions/extension_loader_handler.h (right): https://codereview.chromium.org/342003005/diff/290001/chrome/browser/ui/webui... chrome/browser/ui/webui/extensions/extension_loader_handler.h:52: const GURL& url, On 2014/07/09 23:03:32, Devlin wrote: > nit: put the "implementations" next to each other in the header (and the cc). Done.
Cool! Now let's get some styling in there. :) https://codereview.chromium.org/342003005/diff/310001/chrome/browser/extensio... File chrome/browser/extensions/extension_error_reporter.cc (right): https://codereview.chromium.org/342003005/diff/310001/chrome/browser/extensio... chrome/browser/extensions/extension_error_reporter.cc:93: void ExtensionErrorReporter::AddObserver( Don't know how I've missed this in the last 10 patch sets but... I think this all fits on one line.
On 2014/07/10 21:12:45, Devlin wrote: > Cool! Now let's get some styling in there. :) > > https://codereview.chromium.org/342003005/diff/310001/chrome/browser/extensio... > File chrome/browser/extensions/extension_error_reporter.cc (right): > > https://codereview.chromium.org/342003005/diff/310001/chrome/browser/extensio... > chrome/browser/extensions/extension_error_reporter.cc:93: void > ExtensionErrorReporter::AddObserver( > Don't know how I've missed this in the last 10 patch sets but... > I think this all fits on one line. Went ahead and fixed that AddObserver formatting, along with the HTML formatting to take the span out of the list. And styling. Here's another screenshot: http://i.imgur.com/4jrHKlU.png Seem reasonable? I figured it looked more consistent with the rest of the failure UI.
finnur@chromium.org: Could you take a look at these files when you're back in the office? chrome/browser/resources/extensions/* chrome/browser/ui/webui/extensions/* Thanks!
On 2014/07/11 19:32:17, gpdavis wrote: > On 2014/07/10 21:12:45, Devlin wrote: > > Cool! Now let's get some styling in there. :) > > > > > https://codereview.chromium.org/342003005/diff/310001/chrome/browser/extensio... > > File chrome/browser/extensions/extension_error_reporter.cc (right): > > > > > https://codereview.chromium.org/342003005/diff/310001/chrome/browser/extensio... > > chrome/browser/extensions/extension_error_reporter.cc:93: void > > ExtensionErrorReporter::AddObserver( > > Don't know how I've missed this in the last 10 patch sets but... > > I think this all fits on one line. > > Went ahead and fixed that AddObserver formatting, along with the HTML formatting > to take the span out of the list. And styling. Here's another screenshot: > > http://i.imgur.com/4jrHKlU.png > > Seem reasonable? I figured it looked more consistent with the rest of the > failure UI. Styling looks better, but it's odd that the path for load failures is fully absolute ("/home/gpdavis/...") and the path for extensions load locations in the list uses ~ ("~/..."). Can we clean that up? Also, can you put the images in the CL description? They'll quickly get lost in this sea of comments.
On 2014/07/11 20:33:30, Devlin wrote: > On 2014/07/11 19:32:17, gpdavis wrote: > > On 2014/07/10 21:12:45, Devlin wrote: > > > Cool! Now let's get some styling in there. :) > > > > > > > > > https://codereview.chromium.org/342003005/diff/310001/chrome/browser/extensio... > > > File chrome/browser/extensions/extension_error_reporter.cc (right): > > > > > > > > > https://codereview.chromium.org/342003005/diff/310001/chrome/browser/extensio... > > > chrome/browser/extensions/extension_error_reporter.cc:93: void > > > ExtensionErrorReporter::AddObserver( > > > Don't know how I've missed this in the last 10 patch sets but... > > > I think this all fits on one line. > > > > Went ahead and fixed that AddObserver formatting, along with the HTML > formatting > > to take the span out of the list. And styling. Here's another screenshot: > > > > http://i.imgur.com/4jrHKlU.png > > > > Seem reasonable? I figured it looked more consistent with the rest of the > > failure UI. > > Styling looks better, but it's odd that the path for load failures is fully > absolute ("/home/gpdavis/...") and the path for extensions load locations in the > list uses ~ ("~/..."). Can we clean that up? > > Also, can you put the images in the CL description? They'll quickly get lost in > this sea of comments. Done dealio. Conveniently enough, I just created path_util in another patch that made PrettifyPath public, so this was a very simple change. Also updated the description with a new screenshot.
https://codereview.chromium.org/342003005/diff/350001/chrome/browser/extensio... File chrome/browser/extensions/extension_service.cc (right): https://codereview.chromium.org/342003005/diff/350001/chrome/browser/extensio... chrome/browser/extensions/extension_service.cc:703: ReloadExtensionImpl(extension_id, false /* be_noisy */); Nit: No multi-line comments, use // (after the semicolon). (also above) https://codereview.chromium.org/342003005/diff/350001/chrome/browser/resource... File chrome/browser/resources/extensions/extension_load_error.css (right): https://codereview.chromium.org/342003005/diff/350001/chrome/browser/resource... chrome/browser/resources/extensions/extension_load_error.css:42: #extension-load-error-additional>span { nit: Add space before and after '>' (also below) https://codereview.chromium.org/342003005/diff/350001/chrome/browser/resource... chrome/browser/resources/extensions/extension_load_error.css:48: margin: 3px 0; There's a fairly good chance this won't work with RTL locales. Instead use: -webkit-margin-before -webkit-margin-after ... (instead of left and right, respectively). https://codereview.chromium.org/342003005/diff/350001/chrome/browser/resource... File chrome/browser/resources/extensions/extension_load_error.html (right): https://codereview.chromium.org/342003005/diff/350001/chrome/browser/resource... chrome/browser/resources/extensions/extension_load_error.html:24: </div> I'm not sure I understand this new category. What additional failures are we showing and why is necessary to add a separate category for them? Can we not use one category for all failures? The screenshot (http://i.imgur.com/02tzoUK.png) feels weird to me. Three extensions failed to load but two have no error to show?? https://codereview.chromium.org/342003005/diff/350001/chrome/browser/ui/webui... File chrome/browser/ui/webui/extensions/extension_loader_handler.cc (right): https://codereview.chromium.org/342003005/diff/350001/chrome/browser/ui/webui... chrome/browser/ui/webui/extensions/extension_loader_handler.cc:167: content::WebContentsObserver::Observe(web_ui()->GetWebContents()); Do we need to call Observe(NULL) when we are done? https://codereview.chromium.org/342003005/diff/350001/chrome/browser/ui/webui... chrome/browser/ui/webui/extensions/extension_loader_handler.cc:195: LoadUnpackedExtensionImpl(file_path); This got me thinking... If I have two failures (extensions A, B -- in that order on the failed_paths_ list), what happens if I fix A and retry? Will it only try reloading B? https://codereview.chromium.org/342003005/diff/350001/chrome/browser/ui/webui... chrome/browser/ui/webui/extensions/extension_loader_handler.cc:200: failed_paths_.pop_back(); Similar argument here. Two failures: Should Ignore dismiss and remove one or both? https://codereview.chromium.org/342003005/diff/350001/chrome/browser/ui/webui... File chrome/browser/ui/webui/extensions/extension_loader_handler.h (right): https://codereview.chromium.org/342003005/diff/350001/chrome/browser/ui/webui... chrome/browser/ui/webui/extensions/extension_loader_handler.h:68: // ExtensionErrorReporter::Observer implementation. Prefer shorter version: // ExtensionErrorReporter::Observer: ... over ... // ExtensionErrorReporter::Observer implementation. Same with line 72. https://codereview.chromium.org/342003005/diff/350001/chrome/browser/ui/webui... chrome/browser/ui/webui/extensions/extension_loader_handler.h:105: // failures UI is ready to receive failures. s/the load failures UI/the UI showing load failures/ s/receive failures/show failure notifications/ ? It is also not quite clear to me why you need this variable. Can you document it a bit? https://codereview.chromium.org/342003005/diff/350001/chrome/browser/ui/webui... chrome/browser/ui/webui/extensions/extension_loader_handler.h:106: bool display_ready_; This variable is a bit general (e.g. Display can mean Monitor). Can you find a better name?
Thanks for the review! Let me know if I can clarify any more about this patch. https://codereview.chromium.org/342003005/diff/350001/chrome/browser/extensio... File chrome/browser/extensions/extension_service.cc (right): https://codereview.chromium.org/342003005/diff/350001/chrome/browser/extensio... chrome/browser/extensions/extension_service.cc:703: ReloadExtensionImpl(extension_id, false /* be_noisy */); On 2014/07/14 10:25:58, Finnur wrote: > Nit: No multi-line comments, use // (after the semicolon). > > (also above) No problem. Devlin originally suggested I do it this way; should this commenting style always be avoided? https://codereview.chromium.org/342003005/diff/350001/chrome/browser/resource... File chrome/browser/resources/extensions/extension_load_error.css (right): https://codereview.chromium.org/342003005/diff/350001/chrome/browser/resource... chrome/browser/resources/extensions/extension_load_error.css:42: #extension-load-error-additional>span { On 2014/07/14 10:25:58, Finnur wrote: > nit: Add space before and after '>' > > (also below) Done. https://codereview.chromium.org/342003005/diff/350001/chrome/browser/resource... chrome/browser/resources/extensions/extension_load_error.css:48: margin: 3px 0; On 2014/07/14 10:25:58, Finnur wrote: > There's a fairly good chance this won't work with RTL locales. > > Instead use: > -webkit-margin-before > -webkit-margin-after > > ... (instead of left and right, respectively). Done. https://codereview.chromium.org/342003005/diff/350001/chrome/browser/resource... File chrome/browser/resources/extensions/extension_load_error.html (right): https://codereview.chromium.org/342003005/diff/350001/chrome/browser/resource... chrome/browser/resources/extensions/extension_load_error.html:24: </div> On 2014/07/14 10:25:58, Finnur wrote: > I'm not sure I understand this new category. What additional failures are we > showing and why is necessary to add a separate category for them? Can we not use > one category for all failures? > > The screenshot (http://i.imgur.com/02tzoUK.png) feels weird to me. Three > extensions failed to load but two have no error to show?? This list essentially functions as a stack, where the item on the top of the stack is displayed. When an unpacked installer fails, the UI pops up showing the information about the manifest. If any additional load failures are triggered while this UI is still showing from the first error, then they are pushed into this stack, and the original failure collapses into a list of failed paths. The new failure then has its manifest displayed. This is the case in the screenshot. Invalid three's manifest is showing because it was the last failure to be triggered. If you fix its manifest and reload it, Invalid's failure gets popped off the stack into the UI, so its manifest is displayed. However, if the manifest is still broken, it will immediately trigger another failure, pushing it back onto the stack so that it remains visible. Hitting "Give up" functions as you would expect; it drops the currently displayed failure and pops the next one off the stack into the UI. This is necessary because the UI only exists on the page in one place, and it could get cluttered really quick if we were displaying the manifests of all the extensions that failed to load. If you refresh the extensions page, all the unpacked extensions get reloaded, and they can all send errors to the UI simultaneously. This is why I chose to do it like a stack. Does that make sense? Can you think of a more reasonable way to approach it? I'd be happy to entertain any other possible implementations. https://codereview.chromium.org/342003005/diff/350001/chrome/browser/ui/webui... File chrome/browser/ui/webui/extensions/extension_loader_handler.cc (right): https://codereview.chromium.org/342003005/diff/350001/chrome/browser/ui/webui... chrome/browser/ui/webui/extensions/extension_loader_handler.cc:167: content::WebContentsObserver::Observe(web_ui()->GetWebContents()); On 2014/07/14 10:25:58, Finnur wrote: > Do we need to call Observe(NULL) when we are done? Done with what, exactly? Done with observing for the notification that the UI is ready to receive failures? Observe(NULL) would remove this as an observer, right? This object exists through page refreshes. It's constructed when the page is loaded and destructed when the tab is closed. So this needs to continue observing web contents, because this object needs to continue being notified of page refreshes. https://codereview.chromium.org/342003005/diff/350001/chrome/browser/ui/webui... chrome/browser/ui/webui/extensions/extension_loader_handler.cc:195: LoadUnpackedExtensionImpl(file_path); On 2014/07/14 10:25:58, Finnur wrote: > This got me thinking... If I have two failures (extensions A, B -- in that order > on the failed_paths_ list), what happens if I fix A and retry? Will it only try > reloading B? Yes, that is the current functionality. The "Retry" and "Give up" buttons are for the failure that is currently being displayed. This is why there's a separation with "additional failures". It seemed to me like the user would likely attempt to fix the failure whose manifest is being displayed. Suggestions are more than welcome :) https://codereview.chromium.org/342003005/diff/350001/chrome/browser/ui/webui... chrome/browser/ui/webui/extensions/extension_loader_handler.cc:200: failed_paths_.pop_back(); On 2014/07/14 10:25:58, Finnur wrote: > Similar argument here. Two failures: Should Ignore dismiss and remove one or > both? I guess that's more of a design question for you guys. I would make the argument that "Give up" should only dismiss the failure that's currently being displayed. That way it can be used like a stack, and the failures can be dealt with one by one. https://codereview.chromium.org/342003005/diff/350001/chrome/browser/ui/webui... File chrome/browser/ui/webui/extensions/extension_loader_handler.h (right): https://codereview.chromium.org/342003005/diff/350001/chrome/browser/ui/webui... chrome/browser/ui/webui/extensions/extension_loader_handler.h:68: // ExtensionErrorReporter::Observer implementation. On 2014/07/14 10:25:58, Finnur wrote: > Prefer shorter version: > // ExtensionErrorReporter::Observer: > ... over ... > // ExtensionErrorReporter::Observer implementation. > > Same with line 72. Done. https://codereview.chromium.org/342003005/diff/350001/chrome/browser/ui/webui... chrome/browser/ui/webui/extensions/extension_loader_handler.h:105: // failures UI is ready to receive failures. On 2014/07/14 10:25:58, Finnur wrote: > s/the load failures UI/the UI showing load failures/ > s/receive failures/show failure notifications/ ? > > It is also not quite clear to me why you need this variable. Can you document it > a bit? Done. https://codereview.chromium.org/342003005/diff/350001/chrome/browser/ui/webui... chrome/browser/ui/webui/extensions/extension_loader_handler.h:106: bool display_ready_; On 2014/07/14 10:25:58, Finnur wrote: > This variable is a bit general (e.g. Display can mean Monitor). Can you find a > better name? Understood. How does ui_ready_ sound? That seems less ambiguous to me.
https://codereview.chromium.org/342003005/diff/350001/chrome/browser/extensio... File chrome/browser/extensions/extension_service.cc (right): https://codereview.chromium.org/342003005/diff/350001/chrome/browser/extensio... chrome/browser/extensions/extension_service.cc:703: ReloadExtensionImpl(extension_id, false /* be_noisy */); Documenting params like this is often helpful, and the style guide says you can use either single- or multi-line comments as long as you are consistent. However, in Chromium we've avoided multi-line comments as much as possible, so we should use single-line comments. https://codereview.chromium.org/342003005/diff/350001/chrome/browser/resource... File chrome/browser/resources/extensions/extension_load_error.html (right): https://codereview.chromium.org/342003005/diff/350001/chrome/browser/resource... chrome/browser/resources/extensions/extension_load_error.html:24: </div> Yes, that makes sense. I still think the labeling for the heading ("Additional failures") is not helpful, but I'll add that comment elsewhere (in context). https://codereview.chromium.org/342003005/diff/350001/chrome/browser/ui/webui... File chrome/browser/ui/webui/extensions/extension_loader_handler.cc (right): https://codereview.chromium.org/342003005/diff/350001/chrome/browser/ui/webui... chrome/browser/ui/webui/extensions/extension_loader_handler.cc:167: content::WebContentsObserver::Observe(web_ui()->GetWebContents()); Sounds good. Maybe that warrants a comment in the code? https://codereview.chromium.org/342003005/diff/350001/chrome/browser/ui/webui... chrome/browser/ui/webui/extensions/extension_loader_handler.cc:195: LoadUnpackedExtensionImpl(file_path); I see. That's good. https://codereview.chromium.org/342003005/diff/350001/chrome/browser/ui/webui... chrome/browser/ui/webui/extensions/extension_loader_handler.cc:200: failed_paths_.pop_back(); Yeah, that's fine. https://codereview.chromium.org/342003005/diff/350001/chrome/browser/ui/webui... File chrome/browser/ui/webui/extensions/extension_loader_handler.h (right): https://codereview.chromium.org/342003005/diff/350001/chrome/browser/ui/webui... chrome/browser/ui/webui/extensions/extension_loader_handler.h:106: bool display_ready_; Sure. https://codereview.chromium.org/342003005/diff/370001/chrome/app/generated_re... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/342003005/diff/370001/chrome/app/generated_re... chrome/app/generated_resources.grd:4781: + Additional failures: The problem I have with this label is that it lacks sufficient clarity. When I looked at the screenshot you provided: http://i.imgur.com/02tzoUK.png, the first thing I thought was "Additional failures" is listing other problems it found with Invalid3. But that's not what was intended. I would suggest removing the ambiguity and use a string such as: Other extensions failing to load: What do you think? I'm actually also tempted to change IDS_EXTENSIONS_LOAD_ERROR_HEADING to: Error loading extension ("Extension Load Error" sounds a bit dry)
https://codereview.chromium.org/342003005/diff/350001/chrome/browser/ui/webui... File chrome/browser/ui/webui/extensions/extension_loader_handler.cc (right): https://codereview.chromium.org/342003005/diff/350001/chrome/browser/ui/webui... chrome/browser/ui/webui/extensions/extension_loader_handler.cc:200: failed_paths_.pop_back(); On 2014/07/15 10:38:45, Finnur wrote: > Yeah, that's fine. Just a reminder: There are only multiple failures in the case where: a) The user has multiple unpacked extensions b) Multiple unpacked extensions have load errors c) The user hits refresh on the chrome:extensions page (we can never see multiple errors from a reload or load unpacked command). Since this situation is pretty rare (and since our current mechanism is to pop up multiple modal dialogs [yuck!]), I think we can have something a bit rougher here, as long as it makes sense. (No action necessary - just documenting reasoning for future generations of CLs). https://codereview.chromium.org/342003005/diff/370001/chrome/app/generated_re... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/342003005/diff/370001/chrome/app/generated_re... chrome/app/generated_resources.grd:4781: + Additional failures: On 2014/07/15 10:38:45, Finnur wrote: > The problem I have with this label is that it lacks sufficient clarity. When I > looked at the screenshot you provided: http://i.imgur.com/02tzoUK.png, the first > thing I thought was "Additional failures" is listing other problems it found > with Invalid3. But that's not what was intended. > > I would suggest removing the ambiguity and use a string such as: > Other extensions failing to load: > > What do you think? > > I'm actually also tempted to change IDS_EXTENSIONS_LOAD_ERROR_HEADING to: > Error loading extension > ("Extension Load Error" sounds a bit dry) These both SGTM.
https://codereview.chromium.org/342003005/diff/350001/chrome/browser/extensio... File chrome/browser/extensions/extension_service.cc (right): https://codereview.chromium.org/342003005/diff/350001/chrome/browser/extensio... chrome/browser/extensions/extension_service.cc:703: ReloadExtensionImpl(extension_id, false /* be_noisy */); On 2014/07/15 10:38:44, Finnur wrote: > Documenting params like this is often helpful, and the style guide says you can > use either single- or multi-line comments as long as you are consistent. > However, in Chromium we've avoided multi-line comments as much as possible, so > we should use single-line comments. Understood. I'll stick with single line comments then (y) https://codereview.chromium.org/342003005/diff/350001/chrome/browser/ui/webui... File chrome/browser/ui/webui/extensions/extension_loader_handler.cc (right): https://codereview.chromium.org/342003005/diff/350001/chrome/browser/ui/webui... chrome/browser/ui/webui/extensions/extension_loader_handler.cc:167: content::WebContentsObserver::Observe(web_ui()->GetWebContents()); On 2014/07/15 10:38:45, Finnur wrote: > Sounds good. Maybe that warrants a comment in the code? Will do! https://codereview.chromium.org/342003005/diff/370001/chrome/app/generated_re... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/342003005/diff/370001/chrome/app/generated_re... chrome/app/generated_resources.grd:4781: + Additional failures: On 2014/07/15 17:51:44, Devlin wrote: > On 2014/07/15 10:38:45, Finnur wrote: > > The problem I have with this label is that it lacks sufficient clarity. When I > > looked at the screenshot you provided: http://i.imgur.com/02tzoUK.png, the > first > > thing I thought was "Additional failures" is listing other problems it found > > with Invalid3. But that's not what was intended. > > > > I would suggest removing the ambiguity and use a string such as: > > Other extensions failing to load: > > > > What do you think? > > > > I'm actually also tempted to change IDS_EXTENSIONS_LOAD_ERROR_HEADING to: > > Error loading extension > > ("Extension Load Error" sounds a bit dry) > > These both SGTM. Ah, I see where the confusion came from. That sounds reasonable to me. I'll go ahead and update these.
What I was asked to review LGTM.
lgtm! Good job! :) https://codereview.chromium.org/342003005/diff/390001/chrome/browser/ui/webui... File chrome/browser/ui/webui/extensions/extension_loader_handler.h (right): https://codereview.chromium.org/342003005/diff/390001/chrome/browser/ui/webui... chrome/browser/ui/webui/extensions/extension_loader_handler.h:74: // Add a failure to failures_. If it was a manifest error, |manifest| will nit: |failures_| https://codereview.chromium.org/342003005/diff/390001/chrome/browser/ui/webui... chrome/browser/ui/webui/extensions/extension_loader_handler.h:101: // Set when the chrome://extensions page is fully loaded and the frontend nit: frontend *is* ready https://codereview.chromium.org/342003005/diff/390001/chrome/browser/ui/webui... chrome/browser/ui/webui/extensions/extension_loader_handler.h:102: // ready to receive failures notifications. The page fails to display nit: failure notifications, I think sounds better. Also, let's say "We need this because the page fails...". Otherwise, it sounds like it's saying it can and will happen in production.
The CQ bit was checked by gpdavis.chromium@gmail.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/gpdavis.chromium@gmail.com/342003005/4...
Thanks! :) Glad we got this one to an acceptable state. Thanks for all the feedback, as well. I appreciate your patience with repeated mistakes like brackets after single line ifs-- I think you caught that one enough for me not to forget ;) Cheers! https://codereview.chromium.org/342003005/diff/390001/chrome/browser/ui/webui... File chrome/browser/ui/webui/extensions/extension_loader_handler.h (right): https://codereview.chromium.org/342003005/diff/390001/chrome/browser/ui/webui... chrome/browser/ui/webui/extensions/extension_loader_handler.h:74: // Add a failure to failures_. If it was a manifest error, |manifest| will On 2014/07/16 20:25:10, Devlin wrote: > nit: |failures_| Done. https://codereview.chromium.org/342003005/diff/390001/chrome/browser/ui/webui... chrome/browser/ui/webui/extensions/extension_loader_handler.h:101: // Set when the chrome://extensions page is fully loaded and the frontend On 2014/07/16 20:25:10, Devlin wrote: > nit: frontend *is* ready Done. https://codereview.chromium.org/342003005/diff/390001/chrome/browser/ui/webui... chrome/browser/ui/webui/extensions/extension_loader_handler.h:102: // ready to receive failures notifications. The page fails to display On 2014/07/16 20:25:10, Devlin wrote: > nit: failure notifications, I think sounds better. > > Also, let's say "We need this because the page fails...". Otherwise, it sounds > like it's saying it can and will happen in production. Done.
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: win_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)
The CQ bit was checked by gpdavis.chromium@gmail.com
The CQ bit was unchecked by gpdavis.chromium@gmail.com
The CQ bit was checked by gpdavis.chromium@gmail.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/gpdavis.chromium@gmail.com/342003005/4...
Message was sent while issue was closed.
Change committed as 283852
Message was sent while issue was closed.
@devlin, This bug is related to this patch: https://code.google.com/p/chromium/issues/detail?id=397467 I found the problem-- 'error' became 'reason' in the JavaScript, and these changes didn't reflect that. Making the change in the dictionary setting in the C++ fixed it. What's the protocol for fixing this exactly? Do I just need to get your LGTM and then commit this minor change?
Message was sent while issue was closed.
On 2014/07/25 18:24:48, gpdavis wrote: > @devlin, > > This bug is related to this patch: > https://code.google.com/p/chromium/issues/detail?id=397467 > > I found the problem-- 'error' became 'reason' in the JavaScript, and these > changes didn't reflect that. Making the change in the dictionary setting in the > C++ fixed it. What's the protocol for fixing this exactly? Do I just need to > get your LGTM and then commit this minor change? Generally, yep, that's all that you'll need to do, if we catch it early. If it leaks through into beta or stable, we'd have to merge it to the release branch, but, luckily, this one's not there yet. :) Since I wasn't sure what was happening with this bug (they assigned it to me, even though they suspected this patch - maybe we have a policy of not giving release blockers to external committers? Not sure...) I went ahead and uploaded the fix a couple hours ago. https://codereview.chromium.org/422533002/ Thanks for checking! :)
Message was sent while issue was closed.
On 2014/07/25 18:35:06, Devlin wrote: > On 2014/07/25 18:24:48, gpdavis wrote: > > @devlin, > > > > This bug is related to this patch: > > https://code.google.com/p/chromium/issues/detail?id=397467 > > > > I found the problem-- 'error' became 'reason' in the JavaScript, and these > > changes didn't reflect that. Making the change in the dictionary setting in > the > > C++ fixed it. What's the protocol for fixing this exactly? Do I just need to > > get your LGTM and then commit this minor change? > > Generally, yep, that's all that you'll need to do, if we catch it early. If it > leaks through into beta or stable, we'd have to merge it to the release branch, > but, luckily, this one's not there yet. :) > > Since I wasn't sure what was happening with this bug (they assigned it to me, > even though they suspected this patch - maybe we have a policy of not giving > release blockers to external committers? Not sure...) I went ahead and uploaded > the fix a couple hours ago. > https://codereview.chromium.org/422533002/ > > Thanks for checking! :) Sidenote- we don't usually upload a new patchset to an old patch. Once it goes in, it should be done. (This goes for error fixes, resubmitting a patch that was reverted, etc). It makes it easier to track revision history.
Message was sent while issue was closed.
On 2014/07/25 18:37:53, Devlin wrote: > On 2014/07/25 18:35:06, Devlin wrote: > > On 2014/07/25 18:24:48, gpdavis wrote: > > > @devlin, > > > > > > This bug is related to this patch: > > > https://code.google.com/p/chromium/issues/detail?id=397467 > > > > > > I found the problem-- 'error' became 'reason' in the JavaScript, and these > > > changes didn't reflect that. Making the change in the dictionary setting in > > the > > > C++ fixed it. What's the protocol for fixing this exactly? Do I just need > to > > > get your LGTM and then commit this minor change? > > > > Generally, yep, that's all that you'll need to do, if we catch it early. If > it > > leaks through into beta or stable, we'd have to merge it to the release > branch, > > but, luckily, this one's not there yet. :) > > > > Since I wasn't sure what was happening with this bug (they assigned it to me, > > even though they suspected this patch - maybe we have a policy of not giving > > release blockers to external committers? Not sure...) I went ahead and > uploaded > > the fix a couple hours ago. > > https://codereview.chromium.org/422533002/ > > > > Thanks for checking! :) > > Sidenote- we don't usually upload a new patchset to an old patch. Once it goes > in, it should be done. (This goes for error fixes, resubmitting a patch that > was reverted, etc). It makes it easier to track revision history. Ah, okay, gotcha. I'll keep that in mind. Thanks for taking care of this! I wonder how we missed that earlier. |
