FRP wrappers. "Update" menu item.

src/engine/main.cpp

Index: src/engine/main.cpp
===================================================================
--- a/src/engine/main.cpp
+++ b/src/engine/main.cpp
@@ -40,15 +40,31 @@
     }
   }
   
-  CriticalSection firstRunLock;
-  bool firstRunActionExecuted = false;
-
+  bool updateAvailable;

[Felix Dahlke, 2013/08/06 10:40:12]

I think you should explicitly initialise this to false, as with
firstRunActionExecuted below, even though it's not necessary.

[Wladimir Palant, 2013/08/13 09:19:51]

Not sure why but this variable is again not being initialized in the current
code.

   void UpdateCallback(const std::string res)
   {
-    //TODO: Display UI here
+    if (updateAvailable)
+      return;
+    Dictionary* dictionary = Dictionary::GetInstance();
+    if (res.length() == 0)
+    {
+      std::wstring upToDateText = dictionary->Lookup("updater", "update-already-up-to-date-text");
+      std::wstring upToDateTitle = dictionary->Lookup("updater", "update-already-up-to-date-title");
+      MessageBox(NULL, upToDateText.c_str(), upToDateTitle.c_str(), MB_OK);

[Felix Dahlke, 2013/08/06 10:40:12]

How about MB_ICONINFORMATION?

[Felix Dahlke, 2013/08/09 07:09:27]

Will it still have the OK button? What I meant was MB_OK | MB_ICONEXCLAMATION.

[Wladimir Palant, 2013/08/13 09:19:51]

Please use MessageBoxW explicitly, you are using wstring as parameters.

+    }
+    else
+    {
+      std::wstring errorText = dictionary->Lookup("updater", "update-error-text");
+      std::wstring errorTitle = dictionary->Lookup("updater", "update-error-title");
+      ReplaceString(errorText, L"?1?", ToUtf16String(res));
+      MessageBox(NULL, errorText.c_str(), errorTitle.c_str(), MB_OK);

[Felix Dahlke, 2013/08/06 10:40:12]

How about MB_ICONEXCLAMATION?

[Felix Dahlke, 2013/08/09 07:09:27]

As above, will this still show the OK  button?

[Wladimir Palant, 2013/08/13 09:19:51]

Please use MessageBoxW explicitly, you are using wstring as parameters.

+    }
     return;

[Felix Dahlke, 2013/08/06 10:40:12]

That's not really necessary.

   }
 
+
+  CriticalSection firstRunLock;
+  bool firstRunActionExecuted = false;
   Communication::OutputBuffer HandleRequest(Communication::InputBuffer& request)
   {
     Communication::OutputBuffer response;
@@ -232,6 +248,7 @@
       }
       case Communication::PROC_CHECK_FOR_UPDATES:
       {
+        updateAvailable = false;
         filterEngine->ForceUpdateCheck(UpdateCallback);
         break;
       }
@@ -249,14 +266,9 @@
         }
         break;
       }
-      case Communication::PROC_GET_APP_LOCALE:
-      {
-        response << ToUtf16String(filterEngine->GetAppLocale());
-        break;
-      }
       case Communication::PROC_GET_DOCUMENTATION_LINK:
       {
-        response << ToUtf16String(filterEngine->GetDocumentationLink());
+        response << ToUtf16String(filterEngine->GetPref("documentation_link")->AsString());
         break;
       }
 
@@ -286,6 +298,7 @@
 
   void OnUpdateAvailable(AdblockPlus::JsEnginePtr jsEngine, AdblockPlus::JsValueList& params)
   {
+    updateAvailable = true;

[Felix Dahlke, 2013/08/06 10:40:12]

What about the error below? Shouldn't we try to download the update again next
time in that case?

[Oleksandr, 2013/08/08 14:19:34]

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:

[Felix Dahlke, 2013/08/08 14:29:54]

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.

[Oleksandr, 2013/08/08 17:06:12]

It is empty in what I've seen and based on updater.js.
On 2013/08/08 14:29:54, Felix H. Dahlke wrote:

[Felix Dahlke, 2013/08/09 07:09:27]

Then we should set updateAvailable to true after we check the params size. That
way, we'd actually see the error.

[Oleksandr, 2013/08/09 07:14:49]

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:

[Felix Dahlke, 2013/08/09 07:18:41]

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.

     if (params.size() < 1)
     {
       Debug("updateAvailable event missing URL");