Bug 40350

Summary:

Attempt to speed up builds by reducing header dependencies

Product:

WebKit

Reporter:

Eric Seidel (no email) <eric>

Component:

New Bugs

Assignee:

Eric Seidel (no email) <eric>

Status:

RESOLVED WONTFIX

Severity:

Normal

CC:

abarth, ahmad.saleem792, darin, dbates, ddkilzer, dglazkov, ggaren, jchaffraix, kbalazs, sam, slewis, thakis, webkit.review.bot

Priority:

Version:

528+ (Nightly build)

Hardware:

Mac

OS:

OS X 10.5

Bug Depends on:

3761

Bug Blocks:

Attachments:

Description	Flags
Patch	none
Patch for landing	none
Should now compile for Chromium	levin: review-

Eric Seidel (no email)

Reported 2010-06-08 22:04:41 PDT

Attempt to speed up builds by reducing header dependencies

Attachments
Patch (41.36 KB, patch) 2010-06-08 22:15 PDT, Eric Seidel (no email)	no flags	Details Formatted Diff Diff
Patch for landing (41.99 KB, patch) 2010-06-08 22:44 PDT, Eric Seidel (no email)	no flags	Details Formatted Diff Diff
Should now compile for Chromium (44.16 KB, patch) 2010-06-09 16:04 PDT, Eric Seidel (no email)	levin: review-	Details Formatted Diff Diff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.

Eric Seidel (no email)

Comment 1 2010-06-08 22:15:17 PDT

Created attachment 58211 [details] Patch

Adam Barth

Comment 2 2010-06-08 22:19:01 PDT

Comment on attachment 58211 [details] Patch Ok. Does this actually make the build faster?

Eric Seidel (no email)

Comment 3 2010-06-08 22:22:20 PDT

I believe so. Difficult to test since full builds take 1hr+ on my laptop atm.

Eric Seidel (no email)

Comment 4 2010-06-08 22:44:43 PDT

Created attachment 58213 [details] Patch for landing

WebKit Review Bot

Comment 5 2010-06-08 23:02:24 PDT

Attachment 58211 [details] did not build on chromium: Build output: http://webkit-commit-queue.appspot.com/results/3192058

Eric Seidel (no email)

Comment 6 2010-06-08 23:08:19 PDT

Comment on attachment 58213 [details] Patch for landing I'm going to need the EWS results. Marking back to r?

WebKit Review Bot

Comment 7 2010-06-09 00:18:48 PDT

Attachment 58213 [details] did not build on chromium: Build output: http://webkit-commit-queue.appspot.com/results/3226083

David Levin

Comment 8 2010-06-09 09:02:40 PDT

Comment on attachment 58213 [details] Patch for landing r- due to ews build break.

Eric Seidel (no email)

Comment 9 2010-06-09 11:23:27 PDT

Thanks. :) I'll figure out what header is depending on PassRefPtr and missing the include and upload a new patch.

Eric Seidel (no email)

Comment 10 2010-06-09 16:04:52 PDT

Created attachment 58305 [details] Should now compile for Chromium

Eric Seidel (no email)

Comment 11 2010-06-09 16:05:28 PDT

Comment on attachment 58305 [details] Should now compile for Chromium Waiting for the EWS bots to roll green then this can be r+'d and cq+'d.

WebKit Review Bot

Comment 12 2010-06-09 17:05:55 PDT

Attachment 58305 [details] did not build on chromium: Build output: http://webkit-commit-queue.appspot.com/results/3239091

David Kilzer (:ddkilzer)

Comment 13 2010-06-09 17:43:09 PDT

(In reply to comment #11) > (From update of attachment 58305 [details]) > Waiting for the EWS bots to roll green then this can be r+'d and cq+'d. How did you test for performance regressions? I think the "outlined" method changes should be in a separate patch.

David Levin

Comment 14 2010-06-09 18:20:05 PDT

Comment on attachment 58305 [details] Should now compile for Chromium r- due to ews build break. Plus Dave had a good point about the methods that have been uninlined and possible perf impact -- would be nice to do that in a separate patch.

Balazs Kelemen

Comment 15 2010-06-10 15:15:50 PDT

Personally, what is annoying me about this patch is the comments about includes. Do we really need those? Generally, we have got scarce comments in the code base. I have not seen explanations about why we include something somewhere yet. Actually, I would be very happy to see more comments in webkit to make the code more readable, but I think this kind of comments gives little useful information to the reader.

Eric Seidel (no email)

Comment 16 2011-05-17 10:51:16 PDT

This is a long-abandoned patch and ma no longer be useful.

Ahmad Saleem

Comment 17 2023-06-03 05:50:15 PDT

@Darin - did a lot of similar activities in RenderStyle and now WebPage. So I think it might to be needed anymore. Can we close this? Unless @Darin, you think we can use this or something similar from it.

Darin Adler

Comment 18 2023-06-03 13:45:50 PDT

It’s too old to be useful, yes. We should do this sort of thing, but this patch isn’t where we will start.

Note You need to log in before you can comment on or make changes to this bug.