Issue 1115 - Some yahoo page not correctly shown on IE8 when ABP enabled

src/shared/Utils.cpp

 /*
  * This file is part of Adblock Plus <https://adblockplus.org/>,
  * Copyright (C) 2006-2015 Eyeo GmbH
  *
  * Adblock Plus is free software: you can redistribute it and/or modify
  * it under the terms of the GNU General Public License version 3 as
  * published by the Free Software Foundation.
  *
  * Adblock Plus is distributed in the hope that it will be useful,
  * but WITHOUT ANY WARRANTY; without even the implied warranty of
@@ skipping 129 matching lines @@
 }
 
 void ReplaceString(std::wstring& input, const std::wstring placeholder, const std::wstring replacement)
 {
   size_t replaceStart = input.find(placeholder);
   if (replaceStart != std::string::npos)
   {
     input.replace(replaceStart, placeholder.length(), replacement);
   }
 }
+
+std::wstring GetQueryString(const std::wstring& url)
+{
+  auto questionSignPos = url.find(L'?');
+  if (questionSignPos == std::wstring::npos)
+  {
+    return L"";
+  }

[Eric, 2015/02/02 18:41:58]

If you increment questionSignPos here, you get a (valid) position for the
beginning of the query string. It eliminates the +1 and -1 in the return
statement and makes the code a little easier to follow.

[sergei, 2015/02/12 14:44:06]

done

+  auto endQueryStringPos = url.find(L'#', questionSignPos);
+  if (endQueryStringPos == std::wstring::npos)
+  {
+    endQueryStringPos = url.length();
+  }
+  return url.substr(questionSignPos + 1, endQueryStringPos - questionSignPos - 1);
+}
+
+void SplitString(const std::wstring& value, wchar_t delimiter,

[Eric, 2015/02/02 18:41:58]

A better name would be ForEachToken(), by analogy with 'std::for_each'. When I
see SplitString, I'm expecting something rather returns, say, a vector.

I'd recommend changing the name of tokenHandler, which is pretty generic already
to something completely generic, like 'fn' or 'op' (the first as in
'std::for_each', the second as in 'std::transform'). 

Perhaps even better, just make a token iterator class and call it with
'for_each' instead of the (just one single) call this function below.

[sergei, 2015/02/12 14:44:06]

done.
Why? I've changed them in the header but in the code they reduce the
readability.
I also very like the idea with token iterators but I don't think it's better
here. It's harder to make a mistake working with `ForEachToken(src, delimiter,
tokenHandler)` than with custom iterators. Let's do it later when we really need
it.

+                 const std::function<bool(std::wstring::const_iterator begin, std::wstring::const_iterator end)>& tokenHandler)
+{
+  if (value.empty() || !tokenHandler)

[Eric, 2015/02/02 18:41:58]

As above, 'tokenHandler' is a reference.

+  {
+    return;
+  }

[Eric, 2015/02/02 18:41:58]

I would write the following loop as a 'for' loop. It makes termination clearer.

  for( size_t delimiterPos = 0 ; delimiterPos != std::wstring::npos ;
++delimiterPos )
  {
    size_t prevDelimiterPos = delimiterPos;
    delimiterPos = value.find(delimiter, prevDelimiterPos);
    //...
  }

[sergei, 2015/02/12 14:44:06]

Firstly, the proposed statements in the loop are not correct. Secondly, for-loop
consists from the following steps, 'initialization', 'condition', 'increment'
and 'statement' (`for(initialization, condition, increment){statement();}`).
Taking into account that 'increment' is last three lines of the loop body as
well as we can leave the loop not only because the 'condition' is false, I would
say it's more logical to use while-loop here.

[Eric, 2015/02/12 17:24:41]

Look more closely.
Providing a second condition to leave the loop is part of the point. 

The critique you are making, that there are two ways of terminating the loop, is
also present in the existing code. There's termination if a position reaches the
end or termination if the function argument returns true. There's no difference
there.

The advantage of the for-loop form is that you don't have to read the body of
the loop to verify that the loop terminates.

[sergei, 2015/02/13 15:21:56]

It results in an infinite cycle. Look, at the end `delimiterPos =
value.find(delimiter, prevDelimiterPos);` sets `delimiterPos` to
std::string::npos which is the greatest possible value for an element of type
size_t and then 'increment' step of for-loop is executed, (`++delimiterPos`)
which results into setting `delimiterPos` value to zero (actually, integer
overflow is undefined behaviour), and it starts from the beginning.
So, I still cannot say that the proposed approach is easier to understand.
I agree here, I personally don't like to have exit points in the body of any
loop as well as several exist points from the function, except the cases when we
return the error. I usually create additional boolean variable which indicates
whether the loop should stop or not and the function returns the correct value
only at one place. But as I got feeling that it's considered as something bad
here.

[Eric, 2015/02/13 16:38:22]

While I think the loop could be improved, it does function correctly and there's
no need to belabor the point.

+  std::wstring::size_type delimiterPos = value.find(delimiter);
+  std::wstring::size_type prevDelimiterPos = 0;
+  while(delimiterPos != std::wstring::npos)
+  {
+    if (!tokenHandler(value.begin() + prevDelimiterPos, value.begin() + delimiterPos))
+    {
+      return;
+    }
+    prevDelimiterPos = delimiterPos;
+    ++prevDelimiterPos; // skip the delimiter
+    delimiterPos = value.find(delimiter, prevDelimiterPos);
+  }
+  tokenHandler(value.begin() + prevDelimiterPos, value.end());
+}
+
+void ProcessQueryStringParameters(const std::wstring& queryString,

[Eric, 2015/02/02 18:41:58]

Similiary, 'ForEachQueryParameter()' would be a better name.

[sergei, 2015/02/12 14:44:06]

done.

+                                  const std::function<bool(const std::wstring& name, const std::wstring& value)>& parameterHandler)
+{
+  if (queryString.empty() || !parameterHandler)

[Eric, 2015/02/02 18:41:58]

Why bother with the term "!parameterHandler" when you're passing
'parameterHandler' by reference? Under what circumstances would that term ever
be true?

[sergei, 2015/02/12 14:44:06]

std::function<...> can be an empty object, so before calling it, we should
ensure that it's not empty.

Anyway, I've removed it here and in ForEachToken because it's actually covered
by the test (see
http://codereview.adblockplus.org/5921969115496448/diff/5668600916475904/test...)

[Eric, 2015/02/12 17:24:41]

I had forgotten that you can have a non-empty reference and also a null
function, so this condition is actually meaningful.

Any iteration with a null function is the same as an empty statement. So I'm
withdrawing my original objection. The original behavior, to do nothing if the
function provide is null, is better behavior.
This works too, though now that I've seen both I prefer the original. If you
want to keep the condition out (as with patch set 2), it deserves a document
comment in the header file that this function throws if the function argument is
null.

[sergei, 2015/02/13 15:21:56]

I've added the comments.

+  {
+    return;
+  }
+  auto unparsedParameterHandler = [&parameterHandler](std::wstring::const_iterator begin, std::wstring::const_iterator end)->bool

[Eric, 2015/02/02 18:41:58]

No need for a local variable here; this lambda can be defined directly as a
function argument below.

[sergei, 2015/02/12 14:44:06]

done

+  {
+    auto assignSignIt = std::find(begin, end, L'=');
+    auto valueBeginsAt = assignSignIt != end ? assignSignIt + 1 : assignSignIt;
+    return parameterHandler(std::wstring(begin, assignSignIt), std::wstring(valueBeginsAt, end));
+  };
+  SplitString(queryString, L'&', unparsedParameterHandler);
+}