WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
153554
[webkitdirs] Clarify logic behind is{PortName} functions.
https://bugs.webkit.org/show_bug.cgi?id=153554
Summary
[webkitdirs] Clarify logic behind is{PortName} functions.
Konstantin Tokarev
Reported
2016-01-27 11:52:15 PST
Instead of separate variable $isGtk, $isEfl, and $isWinCairo this patch adds one $portName variable, and all code for port type determination is gathered in one function.
Attachments
Patch
(4.70 KB, patch)
2016-01-27 12:00 PST
,
Konstantin Tokarev
no flags
Details
Formatted Diff
Diff
Patch
(4.72 KB, patch)
2016-01-27 13:10 PST
,
Konstantin Tokarev
no flags
Details
Formatted Diff
Diff
Patch
(4.87 KB, patch)
2016-01-28 03:48 PST
,
Konstantin Tokarev
no flags
Details
Formatted Diff
Diff
Patch
(5.30 KB, patch)
2016-01-28 09:58 PST
,
Konstantin Tokarev
no flags
Details
Formatted Diff
Diff
Patch
(5.50 KB, patch)
2016-01-28 11:37 PST
,
Konstantin Tokarev
no flags
Details
Formatted Diff
Diff
Patch
(6.39 KB, patch)
2016-01-28 11:51 PST
,
Konstantin Tokarev
no flags
Details
Formatted Diff
Diff
Patch
(6.56 KB, patch)
2016-01-29 08:33 PST
,
Konstantin Tokarev
no flags
Details
Formatted Diff
Diff
Patch
(7.28 KB, patch)
2016-02-01 14:02 PST
,
Konstantin Tokarev
no flags
Details
Formatted Diff
Diff
Archive of layout-test-results from ews114 for mac-yosemite
(791.50 KB, application/zip)
2016-02-01 14:57 PST
,
Build Bot
no flags
Details
Patch
(7.71 KB, patch)
2016-02-02 10:34 PST
,
Konstantin Tokarev
no flags
Details
Formatted Diff
Diff
Show Obsolete
(8)
View All
Add attachment
proposed patch, testcase, etc.
Konstantin Tokarev
Comment 1
2016-01-27 12:00:14 PST
Created
attachment 270020
[details]
Patch
Konstantin Tokarev
Comment 2
2016-01-27 13:10:39 PST
Created
attachment 270031
[details]
Patch
Alex Christensen
Comment 3
2016-01-27 14:01:41 PST
Comment on
attachment 270031
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=270031&action=review
> Tools/Scripts/webkitdirs.pm:103 > + IOS => "IOS",
This should probably be iOS.
Darin Adler
Comment 4
2016-01-27 15:28:27 PST
Comment on
attachment 270031
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=270031&action=review
> Tools/ChangeLog:22 > + * Scripts/webkitdirs.pm: > + (determinePortName): > + (portName): > + (isEfl): > + (isGtk): > + (isWinCairo): > + (isAppleMacWebKit): > + (isAppleWinWebKit): > + (isIOSWebKit): > + (cmakeBasedPortName): > + (checkForArgumentAndRemoveFromArrayRef): Deleted. > + (isX86_64): Deleted. > + (isCrossCompilation): Deleted. > + (createiOSSimulatorDevice): Deleted. > + (buildCMakeProjectOrExit): Deleted.
In a good quality change log, we make comments saying what we did to each function.
>> Tools/Scripts/webkitdirs.pm:103 >> + IOS => "IOS", > > This should probably be iOS.
Yes, it should.
> Tools/Scripts/webkitdirs.pm:1060 > + $portName = "AppleWin";
Don’t we want to use the constant here?
> Tools/Scripts/webkitdirs.pm:1064 > + $portName = "IOS";
Don’t we want to use the constant here?
> Tools/Scripts/webkitdirs.pm:1066 > + $portName = "Mac";
Don’t we want to use the constant here?
Konstantin Tokarev
Comment 5
2016-01-28 03:14:10 PST
Comment on
attachment 270031
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=270031&action=review
>> Tools/ChangeLog:22 >> + (buildCMakeProjectOrExit): Deleted. > > In a good quality change log, we make comments saying what we did to each function.
Thanks, will fix it.
>>> Tools/Scripts/webkitdirs.pm:103 >>> + IOS => "IOS", >> >> This should probably be iOS. > > Yes, it should.
I know what is the right capitalization of "iOS", however I capitilized "I" here because other port names start with capital and these names are often used in the middle of word, e.g., isIOSWebKit(), and I guess there will be PlatformIOS.cmake when/if iOS port migrates to cmake. However we can just ucfirst portName() where capitalization is needed.
>> Tools/Scripts/webkitdirs.pm:1060 >> + $portName = "AppleWin"; > > Don’t we want to use the constant here?
Yes, we do.
Konstantin Tokarev
Comment 6
2016-01-28 03:36:59 PST
Comment on
attachment 270031
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=270031&action=review
>>> Tools/ChangeLog:22 >>> + (buildCMakeProjectOrExit): Deleted. >> >> In a good quality change log, we make comments saying what we did to each function. > > Thanks, will fix it.
BTW, prepare-ChangeLog miserably failed to detect deleted functions here.
Konstantin Tokarev
Comment 7
2016-01-28 03:48:05 PST
Created
attachment 270105
[details]
Patch
WebKit Commit Bot
Comment 8
2016-01-28 09:18:46 PST
Comment on
attachment 270105
[details]
Patch Clearing flags on attachment: 270105 Committed
r195742
: <
http://trac.webkit.org/changeset/195742
>
WebKit Commit Bot
Comment 9
2016-01-28 09:18:50 PST
All reviewed patches have been landed. Closing bug.
Chris Dumez
Comment 10
2016-01-28 09:44:12 PST
Reverted
r195742
for reason: Broke EWS Committed
r195746
: <
http://trac.webkit.org/changeset/195746
>
Chris Dumez
Comment 11
2016-01-28 09:44:55 PST
(In reply to
comment #10
)
> Reverted
r195742
for reason: > > Broke EWS > > Committed
r195746
: <
http://trac.webkit.org/changeset/195746
>
Failed to run "['/home/slave/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=ltilve-gtk-ews', 'update', '--port=gtk-wk2']" exit_code: 2 cwd: /home/slave/WebKit Updating working directory Failed to run "['Tools/Scripts/update-webkit']" exit_code: 255 Undefined subroutine &main::determineIsWinCairo called at Tools/Scripts/update-webkit line 46. Failed to run "['Tools/Scripts/update-webkit']" exit_code: 255 Undefined subroutine &main::determineIsWinCairo called at Tools/Scripts/update-webkit line 46. c.f.
https://webkit-queues.webkit.org/results/750792
Konstantin Tokarev
Comment 12
2016-01-28 09:58:54 PST
Created
attachment 270124
[details]
Patch
Chris Dumez
Comment 13
2016-01-28 10:07:23 PST
Our EWS still has not recovered despite the roll out because this patch broke the update-webkit script and thus the bots are unable to update past the rollout :( I think someone is going to have to 'svn up' manually on all these bots :(
Konstantin Tokarev
Comment 14
2016-01-28 11:37:52 PST
Created
attachment 270134
[details]
Patch
Konstantin Tokarev
Comment 15
2016-01-28 11:51:55 PST
Created
attachment 270136
[details]
Patch
Konstantin Tokarev
Comment 16
2016-01-29 08:33:09 PST
Created
attachment 270213
[details]
Patch
Konstantin Tokarev
Comment 17
2016-01-29 08:44:05 PST
New version of patch confesses that we have a state when we don't know which port is chosen but it is not fatal. This situation currently happens on Linux in update-webkit and several other scripts which don't officially accept arguments --gtk or --efl, so both isEfl() and isGtk() are false (and this patch does not change this behavior, it just makes it less subtle). However, build-jsc and build-webkit bail out if port is unknown. I thought about making GTK default port for OS which is not Windows and not Darwin, but I believe that would only make situation worse if people start relying on such behavior, becuase in this case it would be quite possible to have e.g. EFL port run update-webkit without arguments and it would internally assume that it is updating GTK.
Konstantin Tokarev
Comment 18
2016-01-29 08:48:31 PST
I've tested update-webkit and it should not break EWS this time.
Michael Catanzaro
Comment 19
2016-01-29 15:31:01 PST
Comment on
attachment 270213
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=270213&action=review
> Tools/Scripts/build-jsc:86 > + die "Please choose which WebKit port to build";
It would be good to list available ports and show how to select the port. For example: "Please specify which WebKit port to build with --efl or --gtk." This only matters for Linux, since other platforms have default ports. This could could be shared between build-jsc, build-webkit, and also run-minibrowser (which currently prints an extraordinarily confusing warning that the platform is not supported unless you magically know to pass --efl or --gtk).
Darin Adler
Comment 20
2016-01-30 12:00:27 PST
Comment on
attachment 270213
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=270213&action=review
>> Tools/Scripts/build-jsc:86 >> + die "Please choose which WebKit port to build"; > > It would be good to list available ports and show how to select the port. For example: > > "Please specify which WebKit port to build with --efl or --gtk." > > This only matters for Linux, since other platforms have default ports. > > This could could be shared between build-jsc, build-webkit, and also run-minibrowser (which currently prints an extraordinarily confusing warning that the platform is not supported unless you magically know to pass --efl or --gtk).
It’s slightly more idiomatic perl to write: die "Please specify which WebKit port to build with --efl or --gtk." if $isUnknownPort();
> Tools/Scripts/build-webkit:142 > +if (isUnknownPort()) { > + die "Please choose which WebKit port to build"; > +}
Ditto. Quite annoying that this needs to be repeated in two separate scripts. Please find a way to avoid that.
Konstantin Tokarev
Comment 21
2016-02-01 14:02:54 PST
Created
attachment 270424
[details]
Patch
Build Bot
Comment 22
2016-02-01 14:56:58 PST
Comment on
attachment 270424
[details]
Patch
Attachment 270424
[details]
did not pass mac-debug-ews (mac): Output:
http://webkit-queues.webkit.org/results/769419
New failing tests: imported/w3c/web-platform-tests/streams-api/readable-streams/garbage-collection-1.html
Build Bot
Comment 23
2016-02-01 14:57:01 PST
Created
attachment 270430
[details]
Archive of layout-test-results from ews114 for mac-yosemite The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews114 Port: mac-yosemite Platform: Mac OS X 10.10.5
Konstantin Tokarev
Comment 24
2016-02-02 10:34:10 PST
Created
attachment 270498
[details]
Patch
Konstantin Tokarev
Comment 25
2016-02-02 11:22:59 PST
Comment on
attachment 270213
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=270213&action=review
>>> Tools/Scripts/build-jsc:86 >>> + die "Please choose which WebKit port to build"; >> >> It would be good to list available ports and show how to select the port. For example: >> >> "Please specify which WebKit port to build with --efl or --gtk." >> >> This only matters for Linux, since other platforms have default ports. >> >> This could could be shared between build-jsc, build-webkit, and also run-minibrowser (which currently prints an extraordinarily confusing warning that the platform is not supported unless you magically know to pass --efl or --gtk). > > It’s slightly more idiomatic perl to write: > > die "Please specify which WebKit port to build with --efl or --gtk." if $isUnknownPort();
Done, run-minibrowser now dies in the same manner.
>> Tools/Scripts/build-webkit:142 >> +} > > Ditto. > > Quite annoying that this needs to be repeated in two separate scripts. Please find a way to avoid that.
Done.
Konstantin Tokarev
Comment 26
2016-02-02 13:52:39 PST
I think that prohibitUnknownPort would be much more obvious if pased as import parameter of webkitdirs, i.e. use webkitdirs prohibitUnknownPort => 1; but webkitdirs already uses Exporter. I'm pretty sure that Exporter is not really needed for this module (in fact, it is used with import list only in one place right now), but such refactoring would be clearly too much for this patch.
Michael Catanzaro
Comment 27
2016-02-02 15:16:46 PST
Comment on
attachment 270498
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=270498&action=review
> Tools/Scripts/webkitdirs.pm:101 > + Efl => "Efl",
Shouldn't it be EFL in all caps?
> Tools/Scripts/webkitdirs.pm:2113 > + return ucfirst portName();
To make sure we have PlatformIOS and OptionsIOS rather than PlatformiOS and OptionsiOS? Good thinking.
WebKit Commit Bot
Comment 28
2016-02-03 02:46:40 PST
Comment on
attachment 270498
[details]
Patch Rejecting
attachment 270498
[details]
from commit-queue.
annulen@yandex.ru
does not have committer permissions according to
http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/common/config/contributors.json
. - If you do not have committer rights please read
http://webkit.org/coding/contributing.html
for instructions on how to use bugzilla flags. - If you have committer rights please correct the error in Tools/Scripts/webkitpy/common/config/contributors.json by adding yourself to the file (no review needed). The commit-queue restarts itself every 2 hours. After restart the commit-queue will correctly respect your committer rights.
WebKit Commit Bot
Comment 29
2016-02-03 03:38:18 PST
Comment on
attachment 270498
[details]
Patch Clearing flags on attachment: 270498 Committed
r196065
: <
http://trac.webkit.org/changeset/196065
>
WebKit Commit Bot
Comment 30
2016-02-03 03:38:25 PST
All reviewed patches have been landed. Closing bug.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug