Issue 1795 - Add helper std::wstring GetLocationUrl(IWebBrowser2& browser)

[Eric, 2015-01-13 17:04:20]

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

http://codereview.adblockplus.org/5469643829411840/diff/5629499534213120/src/...
src/plugin/PluginUtil.cpp:34: std::wstring retValue;
Use of this variable requires extraneous assignments.

http://codereview.adblockplus.org/5469643829411840/diff/5629499534213120/src/...
src/plugin/PluginUtil.cpp:36: if
(SUCCEEDED(browser.get_LocationURL(&locationUrl)) && !!locationUrl)
No need for double negation. Testing a converted pointer as boolean is
completely reliable, and it's the pattern used for CComBSTR extensively in the
code.

http://codereview.adblockplus.org/5469643829411840/diff/5629499534213120/src/...
src/plugin/PluginUtil.cpp:38: retValue.assign(locationUrl,
locationUrl.Length());
return std::wstring(locationUrl, locationUrl.Length());

http://codereview.adblockplus.org/5469643829411840/diff/5629499534213120/src/...
src/plugin/PluginUtil.cpp:40: return retValue;
return L"";
 -- or --
return std:;wstring();

http://codereview.adblockplus.org/5469643829411840/diff/5629499534213120/src/...
File src/plugin/PluginUtil.h (right):

http://codereview.adblockplus.org/5469643829411840/diff/5629499534213120/src/...
src/plugin/PluginUtil.h:8: std::wstring GetLocationUrl(IWebBrowser2& browser);
I had a version of this function in the string conversion I did last summer (the
one that no one liked). Instead of a collection of loose functions all taking
IWebBrowser2 arguments, this and analogous functions were methods of a wrapper
class. Since you're already using a reference (assuming null checking is
elsewhere), there's no reason not to start off by making this a wrapper class.

[sergei, 2015-01-29 12:42:11]

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

http://codereview.adblockplus.org/5469643829411840/diff/5629499534213120/src/...
src/plugin/PluginUtil.cpp:34: std::wstring retValue;
On 2015/01/13 17:04:20, Eric wrote:
> Use of this variable requires extraneous assignments.

I've changed the body of this function and removed `retValue`.

http://codereview.adblockplus.org/5469643829411840/diff/5629499534213120/src/...
File src/plugin/PluginUtil.h (right):

http://codereview.adblockplus.org/5469643829411840/diff/5629499534213120/src/...
src/plugin/PluginUtil.h:8: std::wstring GetLocationUrl(IWebBrowser2& browser);
On 2015/01/13 17:04:20, Eric wrote:
> I had a version of this function in the string conversion I did last summer
(the
> one that no one liked). Instead of a collection of loose functions all taking
> IWebBrowser2 arguments, this and analogous functions were methods of a wrapper
> class. Since you're already using a reference (assuming null checking is
> elsewhere), there's no reason not to start off by making this a wrapper class.

Should I create the class for it?

[Oleksandr, 2015-01-30 14:29:45]

LGTM

http://codereview.adblockplus.org/5469643829411840/diff/5685265389584384/src/...
File src/plugin/PluginUtil.h (right):

http://codereview.adblockplus.org/5469643829411840/diff/5685265389584384/src/...
src/plugin/PluginUtil.h:26: 
No new string in the end of file, please

[Eric, 2015-02-02 18:44:53]

LGTM

[Eric, 2015-02-02 19:07:54]

http://codereview.adblockplus.org/5469643829411840/diff/5629499534213120/src/...
File src/plugin/PluginUtil.h (right):

http://codereview.adblockplus.org/5469643829411840/diff/5629499534213120/src/...
src/plugin/PluginUtil.h:8: std::wstring GetLocationUrl(IWebBrowser2& browser);
On 2015/01/29 12:42:11, sergei wrote:
> Should I create the class for it?

The next time you write a function with an argument of type 'IBrowser2&', that's
the time to create a class for it and use methods. Not so much need when there's
only one loose function about.