Rename LOG() to LOG_INFO() (issue 13643002)

[dan...@chromium.org]

Reviewers: eseidel, jamesr,

Description:
Rename LOG() to LOG_INFO()

The current macro name conflicts with a macro from the chromium base/
library. This conflict makes it very difficult to include headers from
both Blink, and Chromium, since wtf/Assertions.h is included from just
about anywhere in Blink.

We should switch to using Chromium's logging mechanisms. But until we
do this avoids conflicts between defines.

Moved from: https://bugs.webkit.org/show_bug.cgi?id=101825

R=eseidel,jamesr


Please review this at https://codereview.chromium.org/13643002/

SVN Base: https://chromium.googlesource.com/chromium/blink@master

Affected files:
M Source/WTF/wtf/Assertions.h
M Source/WTF/wtf/RefCountedLeakCounter.cpp
M Source/WebCore/Modules/indexeddb/IDBTracing.h
M Source/WebCore/Modules/webaudio/DefaultAudioDestinationNode.cpp
M Source/WebCore/Modules/webaudio/MediaElementAudioSourceNode.cpp
M Source/WebCore/Modules/webaudio/MediaStreamAudioSourceNode.cpp
M Source/WebCore/Modules/webdatabase/Database.cpp
M Source/WebCore/Modules/webdatabase/DatabaseBackend.cpp
M Source/WebCore/Modules/webdatabase/DatabaseBackendBase.cpp
M Source/WebCore/Modules/webdatabase/DatabaseManager.cpp
M Source/WebCore/Modules/webdatabase/DatabaseTask.cpp
M Source/WebCore/Modules/webdatabase/DatabaseThread.cpp
M Source/WebCore/Modules/webdatabase/DatabaseTracker.cpp
M Source/WebCore/Modules/webdatabase/SQLStatementBackend.cpp
M Source/WebCore/Modules/webdatabase/SQLTransaction.cpp
M Source/WebCore/Modules/webdatabase/SQLTransactionBackend.cpp
M Source/WebCore/Modules/websockets/WebSocket.cpp
M Source/WebCore/Modules/websockets/WebSocketChannel.cpp
M Source/WebCore/Modules/websockets/WebSocketDeflater.cpp
M Source/WebCore/Modules/websockets/WebSocketHandshake.cpp
M Source/WebCore/bridge/testbindings.cpp
M Source/WebCore/bridge/testbindings.mm
M Source/WebCore/dom/Position.cpp
M Source/WebCore/fileapi/FileReader.cpp
M Source/WebCore/fileapi/FileThread.cpp
M Source/WebCore/history/CachedFrame.cpp
M Source/WebCore/history/PageCache.cpp
M Source/WebCore/html/FTPDirectoryDocument.cpp
M Source/WebCore/html/HTMLMediaElement.cpp
M Source/WebCore/html/HTMLPlugInImageElement.cpp
M Source/WebCore/html/HTMLSourceElement.cpp
M Source/WebCore/html/HTMLTrackElement.cpp
M Source/WebCore/html/track/InbandTextTrack.cpp
M Source/WebCore/html/track/TextTrackRegion.cpp
M Source/WebCore/loader/FrameLoader.cpp
M Source/WebCore/loader/HistoryController.cpp
M Source/WebCore/loader/ProgressTracker.cpp
M Source/WebCore/loader/ResourceLoadScheduler.cpp
M Source/WebCore/loader/SubresourceLoader.cpp
M Source/WebCore/loader/TextTrackLoader.cpp
M Source/WebCore/loader/archive/cf/LegacyWebArchive.cpp
M Source/WebCore/loader/cache/CachedResource.cpp
M Source/WebCore/loader/cache/CachedResourceLoader.cpp
M Source/WebCore/loader/cache/MemoryCache.cpp
M Source/WebCore/loader/icon/IconController.cpp
M Source/WebCore/loader/icon/IconDatabase.cpp
M Source/WebCore/loader/icon/IconLoader.cpp
M Source/WebCore/loader/icon/IconRecord.cpp
M Source/WebCore/platform/audio/FFTFrame.cpp
M Source/WebCore/platform/graphics/MediaPlayer.cpp
M Source/WebCore/platform/network/chromium/SocketStreamHandle.cpp
M Source/WebCore/platform/sql/SQLiteDatabase.cpp
M Source/WebCore/platform/sql/SQLiteStatement.cpp
M Source/WebCore/rendering/RenderLayerCompositor.cpp

[ese...@chromium.org]

On 2013/04/04 14:12:18, danakj wrote:

I'm OK with this idea. Do you have a proposal for moving more directly to
Chromium logging? or is this just scratching an old itch?

https://codereview.chromium.org/13643002/

[dan...@chromium.org]

On 2013/04/04 14:36:23, Eric Seidel (Google) wrote:
> On 2013/04/04 14:12:18, danakj wrote:

> I'm OK with this idea. Do you have a proposal for moving more directly to
> Chromium logging? or is this just scratching an old itch?

It's scratching an itch but also I think it is important in general. We've
had
to do some hoop-jumping to include chromium headers like gfx types in
WebKit to
avoid this collision, and if we start trying to include more chromium
headers
from blink to use chromium types, this conflict is going to cause us pain.
IMO
the sooner it goes, the easier it will be to think about including chromium
base/ or ui/gfx/ or whatever else headers.

https://codereview.chromium.org/13643002/

[ese...@chromium.org]

It's possible to argue either way as to which LOG should change, but in the
intrest of expediency, it seems OK for Blink to be the one changing here.
This
is slightly lame, but I understand the goal.

https://codereview.chromium.org/13643002/

[ese...@chromium.org]

lgtm



https://codereview.chromium.org/13643002/

[aba...@chromium.org]

There are some hacks around #undef LOG that we should unwind once this patch
lands.


https://codereview.chromium.org/13643002/

[Dana Jansens]

Thanks. IMO its cleaner to rename here since we may be moving to replace it with base/ if anything, not vice-versa.

I'll look into the undefs as well after, thanks for mentioning that.

[Darin Fisher]

Any time we have a conflict like this, it probably indicates a circular dependency between Chromium and Blink.  We should be careful here.  While Chromium and Blink live in separate repositories, it is risky to add more includes of Chromium code from Blink.  The risk is that we'll make it harder to change Chromium code.  Folks hacking on src/base don't expect to need to worry about other repositories.

[Dana Jansens]

On Fri, Apr 5, 2013 at 12:27 AM, Darin Fisher <da...@chromium.org> wrote:

Any time we have a conflict like this, it probably indicates a circular dependency between Chromium and Blink.  We should be careful here.  While Chromium and Blink live in separate repositories, it is risky to add more includes of Chromium code from Blink.  The risk is that we'll make it harder to change Chromium code.  Folks hacking on src/base don't expect to need to worry about other repositories.

In this case, just having Blink include headers out of chromium would not compile if any of them include base/logging.h, regardless of what the chromium side does (ie not including blink things).

For instance this was a problem in the past including ui/gfx/size.h in WebSize.h in order to do conversion between the two, because other parts of WebKit would include WebSize.h and clobber the LOG define. (We wanted to DCHECK that sizes were not negative, but were just unable to as a result.)

[Darin Fisher]

Ideally, that header should only be seen when WEBKIT_IMPLEMENTATION is undefined, and wtf headers should not be used in that case.  The two shouldn't mix.

Did your experience happen to do with src/skia/ext?

[Dana Jansens]

On Fri, Apr 5, 2013 at 9:48 AM, Darin Fisher <da...@chromium.org> wrote:

Ideally, that header should only be seen when WEBKIT_IMPLEMENTATION is undefined, and wtf headers should not be used in that case.  The two shouldn't mix.

Did your experience happen to do with src/skia/ext?

I just put base/logging.h into ui/gfx/size.h and built chrome.

In file included from ../../third_party/WebKit/Source/WebCore/testing/Internals.cpp:32:

In file included from ../../third_party/WebKit/Source/WebCore/page/Chrome.h:27:

In file included from ../../third_party/WebKit/Source/WebCore/platform/HostWindow.h:29:

In file included from ../../third_party/WebKit/Source/WebCore/platform/Widget.h:35:

In file included from ../../third_party/WebKit/Source/WebCore/platform/chromium/PageClientChromium.h:34:

In file included from ../../third_party/WebKit/Source/Platform/chromium/public/WebScreenInfo.h:34:

In file included from ../../third_party/WebKit/Source/Platform/chromium/public/WebRect.h:40:

In file included from ../../ui/gfx/rect.h:20:

In file included from ../../ui/gfx/size.h:11:

../../base/logging.h:376:9: error: 'LOG' macro redefined [-Werror]

The header is only included if not WEBKIT_IMPLEMENTATION as you suggest, but WTF/Assertions.h is used outside of WEBKIT_IMPLEMENTATION I guess.

Some other files are:

In file included from ../../third_party/WebKit/Source/WebCore/testing/InternalSettings.cpp:34:

In file included from ../../third_party/WebKit/Source/WebCore/inspector/InspectorFrontendClientLocal.cpp:34:

[Darin Fisher]

I don't understand why Internals.cpp is built without WEBKIT_IMPLEMENTATION.  Since it is using WebCore headers, that seems like a mistake.

Anyways, you can see that we could keep pulling on this thread.  We could use the LOG redefinition error to find and fix other "bugs".

I'm guessing that cleanup isn't really what you were looking to do?

[Dana Jansens]

On Fri, Apr 5, 2013 at 10:09 AM, Darin Fisher <da...@chromium.org> wrote:

I don't understand why Internals.cpp is built without WEBKIT_IMPLEMENTATION.  Since it is using WebCore headers, that seems like a mistake.

Anyways, you can see that we could keep pulling on this thread.  We could use the LOG redefinition error to find and fix other "bugs".

I'm guessing that cleanup isn't really what you were looking to do?

OK, it wasn't really clear to me that something was inherently wrong in the Blink code in this scenario. I can look into this some more.

[Dana Jansens]

On Fri, Apr 5, 2013 at 7:25 AM, Dana Jansens <dan...@chromium.org> wrote:

On Fri, Apr 5, 2013 at 10:09 AM, Darin Fisher <da...@chromium.org> wrote:

I don't understand why Internals.cpp is built without WEBKIT_IMPLEMENTATION.  Since it is using WebCore headers, that seems like a mistake.

Anyways, you can see that we could keep pulling on this thread.  We could use the LOG redefinition error to find and fix other "bugs".

I'm guessing that cleanup isn't really what you were looking to do?

OK, it wasn't really clear to me that something was inherently wrong in the Blink code in this scenario. I can look into this some more.

I've done some more investigation into this. The webkit test support code was just missing the WEBKIT_IMPLEMENTATION and I needed to export it down the right gyp path to reach those files. That covers the WebCore/testing/Internals.cpp file.

However, DumpRenderTree makes use of WTF, but also requires to not be considered in WEBKIT_IMPLEMENTATION, as it's a consumer of the Web Platform APIs. So this seems to contradict the assumption that using WTF <=> is WEBKIT_IMPLEMENTATION. Is DRT wrong to be using WTF or to not be WEBKIT_IMPLEMENTATION?

[Darin Fisher]

/cry

I thought there was some effort to move DRT away from using WTF.  Also, I presume that once we move entirely over to content-shell, we'll be able to avoid some WTF usage.

In the short term, one option would be to make it possible to include WebKit API headers where WEBKIT_IMPLEMENTATION is *not* defined, but we also do not assume that we can use src/base/.  In other words, we could turn off auto-conversion to gfx::Rect, etc.  That might be a reasonable compromise in the short term.  Even this might be a lot of work.  I'm not sure how much DRT currently benefits from these conversions.  We have many of them.

-Darin

[Jochen Eisinger]

DRT uses WTF indeed, and it's wrong. I changed only the TestRunner library to not use WTF, as DRT is soon dead anyway.

Note that since DRT builds in component mode, it only uses features from DRT that don't have runtime dependencies, e.g. OwnPtr<>. It should be easy to update the remaining files in DRT to not use wtf by replacing WTF with STL classes.

But then again it's going to be deleted soon.

-jochen

[Darin Fisher]

I just spoke w/ Dana, and she seemed to think that nuking the remaining bits of WTF usage would likely be straightfoward.  Famous last words.  Knock on wood, etc. etc.

-Darin

[Dana Jansens]

On Wed, Apr 10, 2013 at 11:47 AM, Jochen Eisinger <joc...@chromium.org> wrote:

DRT uses WTF indeed, and it's wrong. I changed only the TestRunner library to not use WTF, as DRT is soon dead anyway.

Note that since DRT builds in component mode, it only uses features from DRT that don't have runtime dependencies, e.g. OwnPtr<>. It should be easy to update the remaining files in DRT to not use wtf by replacing WTF with STL classes.

At first I though there was very little use of WTF, but there's more than I hoped (grepping for WTF wasn't as useful as for wtf/).

Most of it is straightforward, until it's not. RefPtr has no stl equivalent, not sure how to replace that reasonably without src/base/. Which would again conflict with LOG. So I'm back to believing we should rename LOG to make the WTF removal possible in the future (for DRT or elsewhere).

[Jochen Eisinger]

On Wed, Apr 10, 2013 at 9:05 PM, Dana Jansens <dan...@chromium.org> wrote:

On Wed, Apr 10, 2013 at 11:47 AM, Jochen Eisinger <joc...@chromium.org> wrote:

DRT uses WTF indeed, and it's wrong. I changed only the TestRunner library to not use WTF, as DRT is soon dead anyway.

Note that since DRT builds in component mode, it only uses features from DRT that don't have runtime dependencies, e.g. OwnPtr<>. It should be easy to update the remaining files in DRT to not use wtf by replacing WTF with STL classes.

At first I though there was very little use of WTF, but there's more than I hoped (grepping for WTF wasn't as useful as for wtf/).

Most of it is straightforward, until it's not. RefPtr has no stl equivalent, not sure how to replace that reasonably without src/base/. Which would again conflict with LOG. So I'm back to believing we should rename LOG to make the WTF removal possible in the future (for DRT or elsewhere).

I would just use src/base:

- it's used in the WebKit public API, so you don't even need to change gyp dependencies

- we're going to delete it soon anyway

[Darin Fisher]

Just be cautious about over-using src/base/.  Each time you do so, you make it harder to evolve src/base/ since Blink is in a separate repository from src/base/.  In other words, if we are just talking about adding a dependency on ref_counted.h and scoped_refptr.h, then it seems OK as a temporary solution.  I guess it really depends on the scope of the work though.

-Darin

[Jochen Eisinger]

cs.chromium.org/RefPtr file:DumpRenderTree/chromium suggests that only the test navigation controller uses this to manage it's navigation entries.

[Dana Jansens]

Ok, I've got a reasonable solution (thanks Darin for the suggestion) to just not define LOG outside of WEBKIT_IMPLEMENTATION, and make sure that WEBKIT_IMPLEMENTATION is properly defined everywhere in WebCore and WebKit. I'll close this CL for now.