First run page triggering

First run page triggering

[Wladimir Palant, 2013-07-11 11:58:10]

http://codereview.adblockplus.org/11043057/diff/1/src/engine/main.cpp
File src/engine/main.cpp (right):

http://codereview.adblockplus.org/11043057/diff/1/src/engine/main.cpp#newcode171
src/engine/main.cpp:171: {
Wrong indentation?

http://codereview.adblockplus.org/11043057/diff/1/src/plugin/AdblockPlusClien...
File src/plugin/AdblockPlusClient.cpp (right):

http://codereview.adblockplus.org/11043057/diff/1/src/plugin/AdblockPlusClien...
src/plugin/AdblockPlusClient.cpp:421: bool CAdblockPlusClient::GetIsFirstRun()
CAdblockPlusClient::IsFirstRun() should be a better name - you are querying a
flag here. Same applies to the call name, should be
Communication::PROC_IS_FIRST_RUN.

http://codereview.adblockplus.org/11043057/diff/1/src/plugin/AdblockPlusClien...
src/plugin/AdblockPlusClient.cpp:427: Communication::InputBuffer response =
CallAdblockPlusEngineProcedure(request);
CallAdblockPlusEngineProcedure(Communication::PROC_GET_IS_FIRST_RUN) will do
here, no need to fill OutputBuffer explicitly here.

http://codereview.adblockplus.org/11043057/diff/1/src/plugin/AdblockPlusClien...
src/plugin/AdblockPlusClient.cpp:428: bool res = false;
Please don't initialize that variable, its initial value is ignored and
overwritten one line below.

http://codereview.adblockplus.org/11043057/diff/1/src/plugin/PluginClass.cpp
File src/plugin/PluginClass.cpp (left):

http://codereview.adblockplus.org/11043057/diff/1/src/plugin/PluginClass.cpp#...
src/plugin/PluginClass.cpp:1737: case WM_LAUNCH_INFO:
You probably want to remove the definition of that message as well.

http://codereview.adblockplus.org/11043057/diff/1/src/plugin/PluginClass.cpp
File src/plugin/PluginClass.cpp (right):

http://codereview.adblockplus.org/11043057/diff/1/src/plugin/PluginClass.cpp#...
src/plugin/PluginClass.cpp:1002: if
(CPluginClient::GetInstance()->GetIsFirstRun())
Is CreateStatusBarPane() really the best place for this code? For example, what
about IE6, don't we want to show the first-run page there? Also, won't this code
run twice on some occasions?

From what I understand, this belongs into CPluginClass::InitObject(). Also, we
need to ensure that this code only runs once, no matter how many processes are
created. That's best to be done in the engine - rename the call into something
like PROC_FIRST_RUN_ACTIONS_NEEDED and have it return true only the first time
it is called (critical section required here).

http://codereview.adblockplus.org/11043057/diff/1/src/plugin/PluginClass.cpp#...
src/plugin/PluginClass.cpp:1024: while ((hr =
GetAsyncBrowser()->get_ReadyState(&readyState) == S_OK) && (readyState <=
READYSTATE_LOADED))
We are opening in a new tab, why do we care about the ready state of the current
page?

http://codereview.adblockplus.org/11043057/diff/1/src/plugin/PluginClass.cpp#...
src/plugin/PluginClass.cpp:1042: DEBUG_ERROR_LOG(hr, PLUGIN_ERROR_NAVIGATION,
PLUGIN_ERROR_NAVIGATION_SETTINGS, "Navigation::Failed")
This should probably be PLUGIN_ERROR_NAVIGATION_WELCOME rather than
PLUGIN_ERROR_NAVIGATION_SETTINGS. Either way, do we really need all these debug
messages? Wouldn't one be sufficient, after 10 failed attempts?

http://codereview.adblockplus.org/11043057/diff/1/src/plugin/PluginClass.cpp#...
src/plugin/PluginClass.cpp:1046: }
Does it really have to be that complicated? No other occasions where we open a
new tab take special precautions. If they are necessary then we probably should
create a helper function to open a tab and use it consistently. If not - well...

http://codereview.adblockplus.org/11043057/diff/1/src/plugin/PluginUtil.cpp
File src/plugin/PluginUtil.cpp (right):

http://codereview.adblockplus.org/11043057/diff/1/src/plugin/PluginUtil.cpp#n...
src/plugin/PluginUtil.cpp:32: return FileUrl(GetDllDir() +
L"html\\templates\\firstrun.html");
This should be firstRun.html even though capitalization normally doesn't matter
on Windows.

[Oleksandr, 2013-07-20 20:22:30]

http://codereview.adblockplus.org/11043057/diff/1/src/plugin/PluginClass.cpp
File src/plugin/PluginClass.cpp (right):

http://codereview.adblockplus.org/11043057/diff/1/src/plugin/PluginClass.cpp#...
src/plugin/PluginClass.cpp:1046: }
This turned out to be unnecessary after all. I've been having issues with
Navigate failing, but it turned out for a completely different reason. All works
fine like this.
On 2013/07/11 11:58:10, Wladimir Palant wrote:
> Does it really have to be that complicated? No other occasions where we open a
> new tab take special precautions. If they are necessary then we probably
should
> create a helper function to open a tab and use it consistently. If not -
well...

[Wladimir Palant, 2013-07-21 11:53:45]

Only minor comments, LGTM if these are addressed.

http://codereview.adblockplus.org/11043057/diff/18001/src/engine/main.cpp
File src/engine/main.cpp (right):

http://codereview.adblockplus.org/11043057/diff/18001/src/engine/main.cpp#new...
src/engine/main.cpp:179: response << !firstRunActionTaken;
How about just response << true? No point obscuring the logic here...

http://codereview.adblockplus.org/11043057/diff/18001/src/engine/main.cpp#new...
src/engine/main.cpp:181: }
What about:

else
  response << false;

Shouldn't we always send a response?

http://codereview.adblockplus.org/11043057/diff/18001/src/plugin/PluginClass.cpp
File src/plugin/PluginClass.cpp (right):

http://codereview.adblockplus.org/11043057/diff/18001/src/plugin/PluginClass....
src/plugin/PluginClass.cpp:1025: hr = GetAsyncBrowser()->Navigate(bTest,
&vFlags, NULL, NULL, NULL);
Wrong indentation here?

[Wladimir Palant, 2013-07-22 06:11:43]

http://codereview.adblockplus.org/11043057/diff/17002/src/plugin/PluginUserSe...
File src/plugin/PluginUserSettings.cpp (right):

http://codereview.adblockplus.org/11043057/diff/17002/src/plugin/PluginUserSe...
src/plugin/PluginUserSettings.cpp:103: return
CString(dictionary->Lookup(std::string(CW2A(section)),
std::string(CW2A(key))).c_str());
Since you didn't address that comment in
http://codereview.adblockplus.org/11013110/ - how about changing CString into
CStringW here to match the return type?

[Oleksandr, 2013-07-22 09:04:51]

Ditched the statusbarasked

http://codereview.adblockplus.org/11043057/diff/11006/src/plugin/AdblockPlusC...
File src/plugin/AdblockPlusClient.h (right):

http://codereview.adblockplus.org/11043057/diff/11006/src/plugin/AdblockPlusC...
src/plugin/AdblockPlusClient.h:74: bool IsFirstRun();
Please never mind this. Just appears here again, due to my internal issues.

http://codereview.adblockplus.org/11043057/diff/11006/src/plugin/PluginClass.cpp
File src/plugin/PluginClass.cpp (left):

http://codereview.adblockplus.org/11043057/diff/11006/src/plugin/PluginClass....
src/plugin/PluginClass.cpp:441: if (!settings->GetStatusBarAsked())
Just removed this if and fixed the formatting

[Felix Dahlke, 2013-07-25 13:53:38]

LGTM

[Felix Dahlke, 2013-07-25 13:56:35]

Gotta revert that, seems like the latest patch set didn't include all changes.
Can you please upload a patch set including all the changes?

[Oleksandr, 2013-07-25 14:03:23]

Uploaded all changes in Patch set #5

[Felix Dahlke, 2013-07-25 14:54:37]

http://codereview.adblockplus.org/11043057/diff/31001/src/engine/main.cpp
File src/engine/main.cpp (right):

http://codereview.adblockplus.org/11043057/diff/31001/src/engine/main.cpp#new...
src/engine/main.cpp:44: bool firstRunActionTaken = false;
"firstRunActionTaken" sounds wrong to my ears, I think "firstRunActionExecuted"
is better. Not a native speaker though, so CMIIW.

http://codereview.adblockplus.org/11043057/diff/31001/src/engine/main.cpp#new...
src/engine/main.cpp:221: case Communication::PROC_IS_FIRST_RUN_ACTION_NEEDED:
How about PROC_IS_FIRST_RUN? The function invoking this is called IsFirstRun()
after all.

http://codereview.adblockplus.org/11043057/diff/31001/src/plugin/PluginUtil.cpp
File src/plugin/PluginUtil.cpp (right):

http://codereview.adblockplus.org/11043057/diff/31001/src/plugin/PluginUtil.c...
src/plugin/PluginUtil.cpp:30: std::wstring UserSettingsFirstRunPageUrl()
More like FirstRunPageFileUrl()? Would also love to not duplicate GetDllDir() +
L"html\\templates\\".

[Oleksandr, 2013-07-26 12:06:52]

On 2013/07/25 14:54:37, Felix H. Dahlke wrote:
> http://codereview.adblockplus.org/11043057/diff/31001/src/engine/main.cpp
> File src/engine/main.cpp (right):
> 
>
http://codereview.adblockplus.org/11043057/diff/31001/src/engine/main.cpp#new...
> src/engine/main.cpp:44: bool firstRunActionTaken = false;
> "firstRunActionTaken" sounds wrong to my ears, I think
"firstRunActionExecuted"
> is better. Not a native speaker though, so CMIIW.
> 
>
http://codereview.adblockplus.org/11043057/diff/31001/src/engine/main.cpp#new...
> src/engine/main.cpp:221: case Communication::PROC_IS_FIRST_RUN_ACTION_NEEDED:
> How about PROC_IS_FIRST_RUN? The function invoking this is called IsFirstRun()
> after all.
> 
>
http://codereview.adblockplus.org/11043057/diff/31001/src/plugin/PluginUtil.cpp
> File src/plugin/PluginUtil.cpp (right):
> 
>
http://codereview.adblockplus.org/11043057/diff/31001/src/plugin/PluginUtil.c...
> src/plugin/PluginUtil.cpp:30: std::wstring UserSettingsFirstRunPageUrl()
> More like FirstRunPageFileUrl()? Would also love to not duplicate GetDllDir()
+
> L"html\\templates\\".

[Oleksandr, 2013-07-26 12:07:01]

http://codereview.adblockplus.org/11043057/diff/31001/src/engine/main.cpp
File src/engine/main.cpp (right):

http://codereview.adblockplus.org/11043057/diff/31001/src/engine/main.cpp#new...
src/engine/main.cpp:221: case Communication::PROC_IS_FIRST_RUN_ACTION_NEEDED:
I would say the PROC_IS_FIRST_RUN would be confusing, since one might think it
is just checking for IsFirstRun. That is not the case, so I wanted to be more
explicit here.

On 2013/07/25 14:54:37, Felix H. Dahlke wrote:
> How about PROC_IS_FIRST_RUN? The function invoking this is called IsFirstRun()
> after all.

[Felix Dahlke, 2013-08-02 10:52:24]

LGTM

http://codereview.adblockplus.org/11043057/diff/31001/src/engine/main.cpp
File src/engine/main.cpp (right):

http://codereview.adblockplus.org/11043057/diff/31001/src/engine/main.cpp#new...
src/engine/main.cpp:221: case Communication::PROC_IS_FIRST_RUN_ACTION_NEEDED:
On 2013/07/26 12:07:01, Oleksandr wrote:
> I would say the PROC_IS_FIRST_RUN would be confusing, since one might think it
> is just checking for IsFirstRun. That is not the case, so I wanted to be more
> explicit here.
> 
> On 2013/07/25 14:54:37, Felix H. Dahlke wrote:
> > How about PROC_IS_FIRST_RUN? The function invoking this is called
IsFirstRun()
> > after all.
> 

Fair enough I guess. Might want to rename IsFirstRun() though.