Bug 176065 – Switch HTMLMediaElement to release logging

• Component: Media
• Status: REOPENED

[Eric Carlson]

Switch HTMLMediaElement to release logging

[Eric Carlson]

Created attachment 319410 [details]
Proposed patch.

[Jer Noble]

Comment on attachment 319410 [details]
Proposed patch.

View in context: https://bugs.webkit.org/attachment.cgi?id=319410&action=review

> Source/WTF/wtf/MediaTime.h:32
> +#include <pal/Logger.h>

Is there a way to do this without pulling the Logger.h header into the MediaTime.h header?  MediaTime.h is included everywhere in WebCore, and this would mean Logger.h is also included everywhere in WebCore.

> Source/WebCore/dom/Document.h:56
> +#include <pal/Logger.h>

Here too. Can't we just forward declare Logger?

[Eric Carlson]

Created attachment 319414 [details]
Updated patch.

[Jer Noble]

Comment on attachment 319414 [details]
Updated patch.

View in context: https://bugs.webkit.org/attachment.cgi?id=319414&action=review

> Source/WebCore/html/HTMLMediaElement.h:41
> +#include <pal/Logger.h>

nit: I assume this is needed for the "private PAL::LogHelper" subclass. Can we pull that out into its own file?

[Eric Carlson]

Created attachment 319443 [details]
Patch for landing.

[Eric Carlson]

Created attachment 319464 [details]
Patch for landing.

[Build Bot]

Attachment 319464 [details] did not pass style-queue:


ERROR: Tools/TestWebKitAPI/Tests/WebCore/Logging.cpp:29:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1 in 12 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Build Bot]

Comment on attachment 319464 [details]
Patch for landing.

Attachment 319464 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/4417119

New failing tests:
media/media-source/media-source-sequence-timestamps.html
media/media-source/media-source-seek-back.html
media/media-source/media-source-overlapping-append.html
media/media-source/media-source-overlapping-decodetime.html
media/media-source/media-source-timeoffset.html

[Build Bot]

Created attachment 319476 [details]
Archive of layout-test-results from ews115 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews115  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Eric Carlson]

Created attachment 319480 [details]
Another patch.

[Eric Carlson]

Created attachment 319482 [details]
Another patch.

[Eric Carlson]

Created attachment 319497 [details]
Patch for landing.

[Eric Carlson]

The Windows failure is unrelated to this patch:
 
C:\cygwin\home\buildbot\WebKit\Source\JavaScriptCore\runtime\JSGlobalObjectFunctions.cpp(137): error C2065: 'U8_MAX_LENGTH': undeclared identifier [C:\cygwin\home\buildbot\WebKit\WebKitBuild\Release\Source\JavaScriptCore\JavaScriptCore.vcxproj]

[Alexey Proskuryakov]

CC Per Arne: is this a flaky build failure on Windows? The patch failed to build, but trunk built successfully after unapplying.

[WebKit Commit Bot]

Comment on attachment 319497 [details]
Patch for landing.

Clearing flags on attachment: 319497

Committed r221445: <http://trac.webkit.org/changeset/221445>

[Per Arne Vollan]

(In reply to Alexey Proskuryakov from comment #14)
> CC Per Arne: is this a flaky build failure on Windows? The patch failed to
> build, but trunk built successfully after unapplying.

I would think so, the build failure seems unrelated.

[Ryan Haddad]

This change broke Sierra Release builds: 
/Volumes/Data/slave/sierra-release/build/Source/WebCore/html/HTMLMediaElement.cpp:193:22: error: static declaration of 'LogMedia' follows non-static declaration

https://build.webkit.org/builders/Apple%20Sierra%20Release%20%28Build%29/builds/4614

[Ryan Haddad]

Reverted r221445 for reason:

This change broke Sierra Release builds.

Committed r221455: <http://trac.webkit.org/changeset/221455>

[Eric Carlson]

Created attachment 319626 [details]
Patch.

[WebKit Commit Bot]

Comment on attachment 319626 [details]
Patch.

Rejecting attachment 319626 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-01', 'validate-changelog', '--check-oops', '--non-interactive', 319626, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

ChangeLog entry in Tools/ChangeLog contains OOPS!.

Full output: http://webkit-queues.webkit.org/results/4428197

[Eric Carlson]

Created attachment 319634 [details]
Patch.

[WebKit Commit Bot]

Comment on attachment 319634 [details]
Patch.

Clearing flags on attachment: 319634

Committed r221494: <http://trac.webkit.org/changeset/221494>

[Joseph Pecoraro]

Comment on attachment 319634 [details]
Patch.

View in context: https://bugs.webkit.org/attachment.cgi?id=319634&action=review

> Source/WebCore/PAL/pal/LoggerHelper.h:48
> +#define THIS Logger::MethodAndPointer(className(), __func__, this)
> +
> +#define ALWAYS_LOG(...)     logger().logAlways(logChannel(), __VA_ARGS__)
> +#define ERROR_LOG(...)      logger().error(logChannel(), __VA_ARGS__)
> +#define WARNING_LOG(...)    logger().warning(logChannel(), __VA_ARGS__)
> +#define NOTICE_LOG(...)     logger().notice(logChannel(), __VA_ARGS__)
> +#define INFO_LOG(...)       logger().info(logChannel(), __VA_ARGS__)
> +#define DEBUG_LOG(...)      logger().debug(logChannel(), __VA_ARGS__)

It seems the first argument of these macros is always the special `THIS` macro. Could it just be inlined in these macros, or do you think there are times we may want to not include it?

[Eric Carlson]

(In reply to Joseph Pecoraro from comment #23)
> Comment on attachment 319634 [details]
> Patch.
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=319634&action=review
> 
> > Source/WebCore/PAL/pal/LoggerHelper.h:48
> > +#define THIS Logger::MethodAndPointer(className(), __func__, this)
> > +
> > +#define ALWAYS_LOG(...)     logger().logAlways(logChannel(), __VA_ARGS__)
> > +#define ERROR_LOG(...)      logger().error(logChannel(), __VA_ARGS__)
> > +#define WARNING_LOG(...)    logger().warning(logChannel(), __VA_ARGS__)
> > +#define NOTICE_LOG(...)     logger().notice(logChannel(), __VA_ARGS__)
> > +#define INFO_LOG(...)       logger().info(logChannel(), __VA_ARGS__)
> > +#define DEBUG_LOG(...)      logger().debug(logChannel(), __VA_ARGS__)
> 
> It seems the first argument of these macros is always the special `THIS`
> macro. Could it just be inlined in these macros, or do you think there are
> times we may want to not include it?

That was the first thing I tried, but it fails on some platforms when you don't add any extra arguments. For example "ALWAYS_LOG()" expands to 

  logger().logAlways(logChannel(), Logger::MethodAndPointer(className(), __func__, this), __VA_ARGS__)

and the compilers used by GTK and wpe generate a compile error when you call a variadic macro with a zero variadic part.

[Eric Carlson]

Plus https://webkit.org/b/176065 to fix a test.

[Eric Carlson]

(In reply to Eric Carlson from comment #25)
> Plus https://webkit.org/b/176065 to fix a test.

By which I meant https://trac.webkit.org/r221500, of course.

[Radar WebKit Bug Importer]

<rdar://problem/34216869>

[Matt Lewis]

(In reply to Per Arne Vollan from comment #16)
> (In reply to Alexey Proskuryakov from comment #14)
> > CC Per Arne: is this a flaky build failure on Windows? The patch failed to
> > build, but trunk built successfully after unapplying.
> 
> I would think so, the build failure seems unrelated.

The failure doesn't seem to be flaky. It has failed the build since the patch landed.

[Matt Lewis]

The error given in the results: https://build.webkit.org/builders/Apple%20Win%20Release%20%28Build%29/builds/4091/steps/compile-webkit/logs/stdio

is 
C:\cygwin\home\buildbot\slave\win-release\build\Source\JavaScriptCore\runtime\JSGlobalObjectFunctions.cpp(137): error C2065: 'U8_MAX_LENGTH': undeclared identifier [C:\cygwin\home\buildbot\slave\win-release\build\WebKitBuild\Release\Source\JavaScriptCore\JavaScriptCore.vcxproj]
  C:\cygwin\home\buildbot\slave\win-release\build\Source\JavaScriptCore\runtime\JSGlobalObjectFunctions.cpp(162): note: see reference to function template instantiation 'JSC::JSValue JSC::encode<LChar>(JSC::ExecState *,const WTF::Bitmap<256,uint32_t> &,const CharacterType *,unsigned int)' being compiled
          with
          [
              CharacterType=LChar
          ]

[WebKit Commit Bot]

Re-opened since this is blocked by bug 176258

[Chris Dumez]

Comment on attachment 319634 [details]
Patch.

View in context: https://bugs.webkit.org/attachment.cgi?id=319634&action=review

> Source/WTF/wtf/MediaTime.cpp:555
> +String MediaTime::toString() const

A few efficiency nit fixes:

> Source/WTF/wtf/MediaTime.cpp:559
> +    builder.append("{");

append('{')

> Source/WTF/wtf/MediaTime.cpp:561
> +        builder.append(String::number(m_timeValue));

Would appendNumber() work?

> Source/WTF/wtf/MediaTime.cpp:562
> +        builder.append("/");

append('/')

> Source/WTF/wtf/MediaTime.cpp:563
> +        builder.append(String::number(m_timeScale));

Would appendNumber() work?

> Source/WTF/wtf/MediaTime.cpp:564
> +        builder.append(" = ");

appendLiteral(" = ");

> Source/WTF/wtf/MediaTime.cpp:566
> +    builder.append(String::number(toDouble()));

Would appendNumber() work?

> Source/WTF/wtf/MediaTime.cpp:567
> +    builder.append("}");

append('}')

[Chris Dumez]

Comment on attachment 319634 [details]
Patch.

View in context: https://bugs.webkit.org/attachment.cgi?id=319634&action=review

> Source/WTF/wtf/MediaTime.h:172
> +namespace PAL {

Isn't it a layer violation to refer to PAL in WTF? MediaTime is used in JSC which technically does not require PAL, right?

[Darin Adler]

Comment on attachment 319634 [details]
Patch.

View in context: https://bugs.webkit.org/attachment.cgi?id=319634&action=review

> Source/WebCore/PAL/pal/LoggerHelper.h:41
> +#define THIS Logger::MethodAndPointer(className(), __func__, this)

This broke the Windows build. The "THIS" macro conflicts with a macro in <combaseapi.h>.

[Darin Adler]

(In reply to Chris Dumez from comment #32)
> Isn't it a layer violation to refer to PAL in WTF?

Yes.

[Eric Carlson]

(In reply to Darin Adler from comment #33)
> Comment on attachment 319634 [details]
> Patch.
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=319634&action=review
> 
> > Source/WebCore/PAL/pal/LoggerHelper.h:41
> > +#define THIS Logger::MethodAndPointer(className(), __func__, this)
> 
> This broke the Windows build. The "THIS" macro conflicts with a macro in
> <combaseapi.h>.

Ugh, Matt bug 176258 to roll this out Friday night but it was never committed. I just r+d that patch and will clean things up here after it lands.

[Eric Carlson]

Created attachment 319865 [details]
Patch for landing.

[Eric Carlson]

Comment on attachment 319634 [details]
Patch.

View in context: https://bugs.webkit.org/attachment.cgi?id=319634&action=review

>> Source/WTF/wtf/MediaTime.cpp:555
>> +String MediaTime::toString() const
> 
> A few efficiency nit fixes:

Thanks for the suggestions, updated.

>> Source/WTF/wtf/MediaTime.h:172
>> +namespace PAL {
> 
> Isn't it a layer violation to refer to PAL in WTF? MediaTime is used in JSC which technically does not require PAL, right?

Oops, I moved the template to platform/graphics/MediaPlayer.h (for now at least).

>>> Source/WebCore/PAL/pal/LoggerHelper.h:41
>>> +#define THIS Logger::MethodAndPointer(className(), __func__, this)
>> 
>> This broke the Windows build. The "THIS" macro conflicts with a macro in <combaseapi.h>.
> 
> Ugh, Matt bug 176258 to roll this out Friday night but it was never committed. I just r+d that patch and will clean things up here after it lands.

Fixed.

[WebKit Commit Bot]

Comment on attachment 319865 [details]
Patch for landing.

Clearing flags on attachment: 319865

Committed r221606: <http://trac.webkit.org/changeset/221606>