Issue 11276031: FRP wrappers. "Update" menu item. (Closed)
|
Created:
Aug. 1, 2013, 2:29 a.m. by Oleksandr Modified:
Aug. 13, 2013, 9:19 a.m. Visibility:
Public. |
DescriptionFRP wrappers. "Update" menu item.
Patch Set 1 #
Total comments: 2
Patch Set 2 : GetAppLocale is now retrieved directly from the browser. Get documentation link change. Manual Upda… #
Total comments: 18
Patch Set 3 : Comments addressed #
Total comments: 3
Patch Set 4 : Handle incorrect parameters for UpdateAvailable as an error #
Total comments: 2
MessagesTotal messages: 12
http://codereview.adblockplus.org/11276031/diff/3001/locales/en.ini File locales/en.ini (right): http://codereview.adblockplus.org/11276031/diff/3001/locales/en.ini#newcode13 locales/en.ini:13: install-question-title=Update Adblock Plus for Internet Explorer You made some unrelated changes and mixed the order up a bit here. I reverted that already yesterday. http://codereview.adblockplus.org/11276031/diff/3001/src/engine/main.cpp File src/engine/main.cpp (right): http://codereview.adblockplus.org/11276031/diff/3001/src/engine/main.cpp#newc... src/engine/main.cpp:43: bool updateAvailable; I think you should explicitly initialise this to false, as with firstRunActionExecuted below, even though it's not necessary. http://codereview.adblockplus.org/11276031/diff/3001/src/engine/main.cpp#newc... src/engine/main.cpp:53: MessageBox(NULL, upToDateText.c_str(), upToDateTitle.c_str(), MB_OK); How about MB_ICONINFORMATION? http://codereview.adblockplus.org/11276031/diff/3001/src/engine/main.cpp#newc... src/engine/main.cpp:60: MessageBox(NULL, errorText.c_str(), errorTitle.c_str(), MB_OK); How about MB_ICONEXCLAMATION? http://codereview.adblockplus.org/11276031/diff/3001/src/engine/main.cpp#newc... src/engine/main.cpp:62: return; That's not really necessary. http://codereview.adblockplus.org/11276031/diff/3001/src/engine/main.cpp#newc... src/engine/main.cpp:301: updateAvailable = true; What about the error below? Shouldn't we try to download the update again next time in that case?
http://codereview.adblockplus.org/11276031/diff/3001/src/engine/main.cpp File src/engine/main.cpp (right): http://codereview.adblockplus.org/11276031/diff/3001/src/engine/main.cpp#newc... src/engine/main.cpp:301: updateAvailable = true; hm.. When would be the next time? On automatic update? Does that require any extra steps to be taken here? On 2013/08/06 10:40:12, Felix H. Dahlke wrote: > What about the error below? Shouldn't we try to download the update again next > time in that case?
http://codereview.adblockplus.org/11276031/diff/3001/src/engine/main.cpp File src/engine/main.cpp (right): http://codereview.adblockplus.org/11276031/diff/3001/src/engine/main.cpp#newc... src/engine/main.cpp:301: updateAvailable = true; On 2013/08/08 14:19:34, Oleksandr wrote: > hm.. When would be the next time? On automatic update? Does that require any > extra steps to be taken here? > > On 2013/08/06 10:40:12, Felix H. Dahlke wrote: > > What about the error below? Shouldn't we try to download the update again next > > time in that case? > I got this a bit wrong, it'll check for updates and download updates regardless of what the value of updateAvailable is. If we don't set it to true in the case of the error below, we would show either the "no update available" or the "error while updating" message in UpdateCallback. The latter would be correct, but I'm not sure if res would be empty in this case.
http://codereview.adblockplus.org/11276031/diff/3001/src/engine/main.cpp File src/engine/main.cpp (right): http://codereview.adblockplus.org/11276031/diff/3001/src/engine/main.cpp#newc... src/engine/main.cpp:301: updateAvailable = true; It is empty in what I've seen and based on updater.js. On 2013/08/08 14:29:54, Felix H. Dahlke wrote: > On 2013/08/08 14:19:34, Oleksandr wrote: > > hm.. When would be the next time? On automatic update? Does that require any > > extra steps to be taken here? > > > > On 2013/08/06 10:40:12, Felix H. Dahlke wrote: > > > What about the error below? Shouldn't we try to download the update again > next > > > time in that case? > > > > I got this a bit wrong, it'll check for updates and download updates regardless > of what the value of updateAvailable is. > > If we don't set it to true in the case of the error below, we would show either > the "no update available" or the "error while updating" message in > UpdateCallback. The latter would be correct, but I'm not sure if res would be > empty in this case.
http://codereview.adblockplus.org/11276031/diff/3001/src/engine/main.cpp File src/engine/main.cpp (right): http://codereview.adblockplus.org/11276031/diff/3001/src/engine/main.cpp#newc... src/engine/main.cpp:53: MessageBox(NULL, upToDateText.c_str(), upToDateTitle.c_str(), MB_OK); On 2013/08/06 10:40:12, Felix H. Dahlke wrote: > How about MB_ICONINFORMATION? Will it still have the OK button? What I meant was MB_OK | MB_ICONEXCLAMATION. http://codereview.adblockplus.org/11276031/diff/3001/src/engine/main.cpp#newc... src/engine/main.cpp:60: MessageBox(NULL, errorText.c_str(), errorTitle.c_str(), MB_OK); On 2013/08/06 10:40:12, Felix H. Dahlke wrote: > How about MB_ICONEXCLAMATION? As above, will this still show the OK button? http://codereview.adblockplus.org/11276031/diff/3001/src/engine/main.cpp#newc... src/engine/main.cpp:301: updateAvailable = true; On 2013/08/08 17:06:12, Oleksandr wrote: > It is empty in what I've seen and based on updater.js. > On 2013/08/08 14:29:54, Felix H. Dahlke wrote: > > On 2013/08/08 14:19:34, Oleksandr wrote: > > > hm.. When would be the next time? On automatic update? Does that require any > > > extra steps to be taken here? > > > > > > On 2013/08/06 10:40:12, Felix H. Dahlke wrote: > > > > What about the error below? Shouldn't we try to download the update again > > next > > > > time in that case? > > > > > > > I got this a bit wrong, it'll check for updates and download updates > regardless > > of what the value of updateAvailable is. > > > > If we don't set it to true in the case of the error below, we would show > either > > the "no update available" or the "error while updating" message in > > UpdateCallback. The latter would be correct, but I'm not sure if res would be > > empty in this case. > Then we should set updateAvailable to true after we check the params size. That way, we'd actually see the error.
http://codereview.adblockplus.org/11276031/diff/3001/src/engine/main.cpp File src/engine/main.cpp (right): http://codereview.adblockplus.org/11276031/diff/3001/src/engine/main.cpp#newc... src/engine/main.cpp:301: updateAvailable = true; Hm. But it is set to true at one line above.. What could have happened to it since then? On 2013/08/09 07:09:27, Felix H. Dahlke wrote: > On 2013/08/08 17:06:12, Oleksandr wrote: > > It is empty in what I've seen and based on updater.js. > > On 2013/08/08 14:29:54, Felix H. Dahlke wrote: > > > On 2013/08/08 14:19:34, Oleksandr wrote: > > > > hm.. When would be the next time? On automatic update? Does that require > any > > > > extra steps to be taken here? > > > > > > > > On 2013/08/06 10:40:12, Felix H. Dahlke wrote: > > > > > What about the error below? Shouldn't we try to download the update > again > > > next > > > > > time in that case? > > > > > > > > > > I got this a bit wrong, it'll check for updates and download updates > > regardless > > > of what the value of updateAvailable is. > > > > > > If we don't set it to true in the case of the error below, we would show > > either > > > the "no update available" or the "error while updating" message in > > > UpdateCallback. The latter would be correct, but I'm not sure if res would > be > > > empty in this case. > > > > Then we should set updateAvailable to true after we check the params size. That > way, we'd actually see the error.
http://codereview.adblockplus.org/11276031/diff/3001/src/engine/main.cpp File src/engine/main.cpp (right): http://codereview.adblockplus.org/11276031/diff/3001/src/engine/main.cpp#newc... src/engine/main.cpp:301: updateAvailable = true; On 2013/08/09 07:14:49, Oleksandr wrote: > Hm. But it is set to true at one line above.. What could have happened to it > since then? > > On 2013/08/09 07:09:27, Felix H. Dahlke wrote: > > On 2013/08/08 17:06:12, Oleksandr wrote: > > > It is empty in what I've seen and based on updater.js. > > > On 2013/08/08 14:29:54, Felix H. Dahlke wrote: > > > > On 2013/08/08 14:19:34, Oleksandr wrote: > > > > > hm.. When would be the next time? On automatic update? Does that require > > any > > > > > extra steps to be taken here? > > > > > > > > > > On 2013/08/06 10:40:12, Felix H. Dahlke wrote: > > > > > > What about the error below? Shouldn't we try to download the update > > again > > > > next > > > > > > time in that case? > > > > > > > > > > > > > I got this a bit wrong, it'll check for updates and download updates > > > regardless > > > > of what the value of updateAvailable is. > > > > > > > > If we don't set it to true in the case of the error below, we would show > > > either > > > > the "no update available" or the "error while updating" message in > > > > UpdateCallback. The latter would be correct, but I'm not sure if res would > > be > > > > empty in this case. > > > > > > > Then we should set updateAvailable to true after we check the params size. > That > > way, we'd actually see the error. > Um, the idea was to move that line below the error check, not duplicate it. Let's go through a case that's problematic right now: 1. We check for an update 2. OnUpdateAvailable is called and updateAvailable is set to true 3. params.size() is 0, so we return in the condition below. 4. UpdateCallback is called, but immediately exited since updateAvailable is true. That means the user will not see an error, even though there was one. That's at least how I understand this.
http://codereview.adblockplus.org/11276031/diff/5014/src/engine/Main.cpp File src/engine/Main.cpp (right): http://codereview.adblockplus.org/11276031/diff/5014/src/engine/Main.cpp#newc... src/engine/Main.cpp:331: updateAvailable = true; Probably the most discussed line of code
LGTM http://codereview.adblockplus.org/11276031/diff/5014/src/engine/Main.cpp File src/engine/Main.cpp (right): http://codereview.adblockplus.org/11276031/diff/5014/src/engine/Main.cpp#newc... src/engine/Main.cpp:331: updateAvailable = true; On 2013/08/09 08:17:39, Oleksandr wrote: > Probably the most discussed line of code Quite definitely :D
It seems that Patch Set 3 didn't make it into the repository. http://codereview.adblockplus.org/11276031/diff/1/html/static/js/ieFirstRun.js File html/static/js/ieFirstRun.js (right): http://codereview.adblockplus.org/11276031/diff/1/html/static/js/ieFirstRun.j... html/static/js/ieFirstRun.js:49: Utils.appLocale = Settings.GetAppLocale(); This seems to assume that the result of require("prefs") is assigned to a global Prefs variable where it can be accessed. Also, there is an assumption that this result won't be used before the page load event. I guess that the problem here is that the Settings object isn't available while the page is loading? Then we cannot help the objects being uninitialized while the page is being loaded. However, assuming global variables is completely unnecessary. Please consider the following approach: var AdblockPlus = (function() { var scopes = { prefs: { Prefs: { documentation_link: "" } }, ... }; var result = { require: function(module) { return scopes[module]; } }; window.addEventListener("load", function() { result.getMessage = Settings.GetMessage; scopes.prefs.Prefs.documentation_link = Settings.GetDocumentationLink(); ... }, false); return result; })(); Wrapping everything inside a function ensures that only the AdblockPlus variable is touched in the global scope - all other variables are defined locally and can be accessed from that function only. http://codereview.adblockplus.org/11276031/diff/1/src/shared/Communication.h File src/shared/Communication.h (right): http://codereview.adblockplus.org/11276031/diff/1/src/shared/Communication.h#... src/shared/Communication.h:31: PROC_GET_APP_LOCALE, This is no longer being used but hasn't been removed. http://codereview.adblockplus.org/11276031/diff/3001/src/engine/main.cpp File src/engine/main.cpp (right): http://codereview.adblockplus.org/11276031/diff/3001/src/engine/main.cpp#newc... src/engine/main.cpp:43: bool updateAvailable; On 2013/08/06 10:40:12, Felix H. Dahlke wrote: > I think you should explicitly initialise this to false, as with > firstRunActionExecuted below, even though it's not necessary. Not sure why but this variable is again not being initialized in the current code. http://codereview.adblockplus.org/11276031/diff/3001/src/engine/main.cpp#newc... src/engine/main.cpp:53: MessageBox(NULL, upToDateText.c_str(), upToDateTitle.c_str(), MB_OK); Please use MessageBoxW explicitly, you are using wstring as parameters. http://codereview.adblockplus.org/11276031/diff/3001/src/engine/main.cpp#newc... src/engine/main.cpp:60: MessageBox(NULL, errorText.c_str(), errorTitle.c_str(), MB_OK); Please use MessageBoxW explicitly, you are using wstring as parameters. http://codereview.adblockplus.org/11276031/diff/3001/src/plugin/PluginClass.cpp File src/plugin/PluginClass.cpp (right): http://codereview.adblockplus.org/11276031/diff/3001/src/plugin/PluginClass.c... src/plugin/PluginClass.cpp:1396: ctext = dictionary->Lookup("menu", "menu-update"); What's the point of prefixing everything in the "menu" section with "menu-"? http://codereview.adblockplus.org/11276031/diff/10001/src/engine/main.cpp File src/engine/main.cpp (left): http://codereview.adblockplus.org/11276031/diff/10001/src/engine/main.cpp#old... src/engine/main.cpp:62: return; This change isn't present in the current code. http://codereview.adblockplus.org/11276031/diff/10001/src/engine/main.cpp File src/engine/main.cpp (right): http://codereview.adblockplus.org/11276031/diff/10001/src/engine/main.cpp#new... src/engine/main.cpp:53: MessageBox(NULL, upToDateText.c_str(), upToDateTitle.c_str(), MB_ICONINFORMATION); Not sure why but this change isn't present in the current code. But I agree with Felix, it should be MB_OK | MB_ICONINFORMATION. http://codereview.adblockplus.org/11276031/diff/10001/src/engine/main.cpp#new... src/engine/main.cpp:60: MessageBox(NULL, errorText.c_str(), errorTitle.c_str(), MB_ICONEXCLAMATION); Not sure why but this change isn't present in the current code. But I agree with Felix, it should be MB_OK | MB_ICONEXCLAMATION. 
|
