Moving ICU dependent function ReadUTFChar into the icu.cc file....

Moving ICU dependent function ReadUTFChar into the icu.cc file.
This avoids icu dependency in other object files that use inline functions from url_canon_internal.h.

[brettw, 2009-06-01 19:29:11]

What was the inspiration of this change?

The current organization is that the non _icu.cc files only depend on ICU macros
which you can presumably provide your own implemenation without a lot of effort.

It does seem nice if this was separated, there are quite a number of other ICU
macros in that file that you didn't move. If we're going to do this
reorganization, I think it should be complete rather than having a random
function implemented in another file. I also suspect the complete refactor isn't
worth the effort.

[tommi (sloooow) - chröme, 2009-06-01 19:58:11]

I narrowed the icu dependency in url_canon_internal.h down to U8_NEXT in
ReadUTFChar. The other icu macros are not pulling in actual icu dependencies. 
The U8_NEXT macro that pulls in utf8_nextCharSafeBody whereas the other macros
like U16_GET_SUPPLEMENTARY are just shifting bits etc.

[brettw, 2009-06-01 20:00:59]

http://codereview.chromium.org/118062/diff/1/2
File src/url_canon_internal.h (left):

http://codereview.chromium.org/118062/diff/1/2#oldcode166
Line 166: inline bool ReadUTFChar(const char* str, int* begin, int length,
I think you should also move the UTF-16 version so they're the same.

http://codereview.chromium.org/118062/diff/1/2
File src/url_canon_internal.h (right):

http://codereview.chromium.org/118062/diff/1/2#newcode42
Line 42: #include "googleurl/src/url_canon_icu.h"  // ReadUTFChar
The way this was done was to try to make it easy to replace ICU. Including the
ICU header here doesn't help :)

What I did for the IDN function was to declare it in the general file, and then
provide the implementation in the _icu.cc file. This way you can just choose not
to link the _icu.cc file and you don't have ICU.

So I would do that here as well, and declare the two functions in this header
where they were originally. Probably some comments in each place about where the
corresponding decl/impl is would be helpful.

[tommi (sloooow) - chröme, 2009-06-01 21:07:50]

http://codereview.chromium.org/118062/diff/1/2
File src/url_canon_internal.h (left):

http://codereview.chromium.org/118062/diff/1/2#oldcode166
Line 166: inline bool ReadUTFChar(const char* str, int* begin, int length,
On 2009/06/01 20:01:00, brettw wrote:
> I think you should also move the UTF-16 version so they're the same.

Done.

http://codereview.chromium.org/118062/diff/1/2
File src/url_canon_internal.h (right):

http://codereview.chromium.org/118062/diff/1/2#newcode42
Line 42: #include "googleurl/src/url_canon_icu.h"  // ReadUTFChar
On 2009/06/01 20:01:00, brettw wrote:
> The way this was done was to try to make it easy to replace ICU. Including the
> ICU header here doesn't help :)
> 
> What I did for the IDN function was to declare it in the general file, and
then
> provide the implementation in the _icu.cc file. This way you can just choose
not
> to link the _icu.cc file and you don't have ICU.
> 
> So I would do that here as well, and declare the two functions in this header
> where they were originally. Probably some comments in each place about where
the
> corresponding decl/impl is would be helpful.

Done.

[tommi (sloooow) - chröme, 2009-06-02 18:08:36]

am I good to go?

[brettw, 2009-06-02 18:10:55]

LGTM

[brettw, 2009-06-02 18:21:15]

Committed as googleurl@105.