Bug 141133

Summary:

Use more references in HistoryItem

Product:

WebKit

Reporter:

Chris Dumez <cdumez>

Component:

Page Loading

Assignee:

Chris Dumez <cdumez>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

commit-queue, japhet, kling

Priority:

Version:

528+ (Nightly build)

Hardware:

Unspecified

OS:

Unspecified

Attachments:

Description	Flags
Patch	none
Patch	none
Patch	none
Patch	none
Patch	none
Patch	none
Patch	none
Patch	none
Patch	none

Chris Dumez

Reported 2015-01-31 21:41:59 PST

Use more references in HistoryItem instead of pointers.

Attachments
Patch (42.57 KB, patch) 2015-01-31 22:22 PST, Chris Dumez	no flags	Details Formatted Diff Diff
Patch (43.75 KB, patch) 2015-01-31 22:33 PST, Chris Dumez	no flags	Details Formatted Diff Diff
Patch (45.43 KB, patch) 2015-01-31 23:25 PST, Chris Dumez	no flags	Details Formatted Diff Diff
Patch (45.76 KB, patch) 2015-01-31 23:35 PST, Chris Dumez	no flags	Details Formatted Diff Diff
Patch (47.98 KB, patch) 2015-02-01 00:15 PST, Chris Dumez	no flags	Details Formatted Diff Diff
Patch (49.16 KB, patch) 2015-02-01 00:20 PST, Chris Dumez	no flags	Details Formatted Diff Diff
Patch (49.88 KB, patch) 2015-02-01 00:49 PST, Chris Dumez	no flags	Details Formatted Diff Diff
Patch (51.56 KB, patch) 2015-02-01 14:02 PST, Chris Dumez	no flags	Details Formatted Diff Diff
Patch (51.72 KB, patch) 2015-02-01 15:28 PST, Chris Dumez	no flags	Details Formatted Diff Diff
Show Obsolete (8) View All Add attachment proposed patch, testcase, etc.

Chris Dumez

Comment 1 2015-01-31 22:22:15 PST

Created attachment 245818 [details] Patch

WebKit Commit Bot

Comment 2 2015-01-31 22:24:44 PST

Attachment 245818 [details] did not pass style-queue: ERROR: Source/WebCore/history/HistoryItem.cpp:478: Multi line control clauses should use braces. [whitespace/braces] [4] ERROR: Source/WebCore/history/HistoryItem.cpp:498: Multi line control clauses should use braces. [whitespace/braces] [4] Total errors found: 2 in 20 files If any of these errors are false positives, please file a bug against check-webkit-style.

Chris Dumez

Comment 3 2015-01-31 22:33:09 PST

Created attachment 245819 [details] Patch

WebKit Commit Bot

Comment 4 2015-01-31 22:35:53 PST

Attachment 245819 [details] did not pass style-queue: ERROR: Source/WebCore/history/HistoryItem.cpp:478: Multi line control clauses should use braces. [whitespace/braces] [4] ERROR: Source/WebCore/history/HistoryItem.cpp:498: Multi line control clauses should use braces. [whitespace/braces] [4] Total errors found: 2 in 22 files If any of these errors are false positives, please file a bug against check-webkit-style.

Chris Dumez

Comment 5 2015-01-31 23:25:12 PST

Created attachment 245820 [details] Patch

WebKit Commit Bot

Comment 6 2015-01-31 23:27:23 PST

Attachment 245820 [details] did not pass style-queue: ERROR: Source/WebCore/history/HistoryItem.cpp:478: Multi line control clauses should use braces. [whitespace/braces] [4] ERROR: Source/WebCore/history/HistoryItem.cpp:498: Multi line control clauses should use braces. [whitespace/braces] [4] Total errors found: 2 in 22 files If any of these errors are false positives, please file a bug against check-webkit-style.

Chris Dumez

Comment 7 2015-01-31 23:35:06 PST

Created attachment 245821 [details] Patch

WebKit Commit Bot

Comment 8 2015-01-31 23:36:39 PST

Attachment 245821 [details] did not pass style-queue: ERROR: Source/WebCore/history/HistoryItem.cpp:478: Multi line control clauses should use braces. [whitespace/braces] [4] ERROR: Source/WebCore/history/HistoryItem.cpp:498: Multi line control clauses should use braces. [whitespace/braces] [4] Total errors found: 2 in 22 files If any of these errors are false positives, please file a bug against check-webkit-style.

Chris Dumez

Comment 9 2015-02-01 00:15:39 PST

Created attachment 245822 [details] Patch

WebKit Commit Bot

Comment 10 2015-02-01 00:18:11 PST

Attachment 245822 [details] did not pass style-queue: ERROR: Source/WebCore/history/HistoryItem.cpp:478: Multi line control clauses should use braces. [whitespace/braces] [4] ERROR: Source/WebCore/history/HistoryItem.cpp:498: Multi line control clauses should use braces. [whitespace/braces] [4] Total errors found: 2 in 25 files If any of these errors are false positives, please file a bug against check-webkit-style.

Chris Dumez

Comment 11 2015-02-01 00:20:20 PST

Created attachment 245823 [details] Patch

WebKit Commit Bot

Comment 12 2015-02-01 00:23:13 PST

Attachment 245823 [details] did not pass style-queue: ERROR: Source/WebCore/history/HistoryItem.cpp:478: Multi line control clauses should use braces. [whitespace/braces] [4] ERROR: Source/WebCore/history/HistoryItem.cpp:498: Multi line control clauses should use braces. [whitespace/braces] [4] Total errors found: 2 in 25 files If any of these errors are false positives, please file a bug against check-webkit-style.

Chris Dumez

Comment 13 2015-02-01 00:49:21 PST

Created attachment 245825 [details] Patch

WebKit Commit Bot

Comment 14 2015-02-01 00:50:49 PST

Attachment 245825 [details] did not pass style-queue: ERROR: Source/WebCore/history/HistoryItem.cpp:478: Multi line control clauses should use braces. [whitespace/braces] [4] ERROR: Source/WebCore/history/HistoryItem.cpp:498: Multi line control clauses should use braces. [whitespace/braces] [4] Total errors found: 2 in 25 files If any of these errors are false positives, please file a bug against check-webkit-style.

Andreas Kling

Comment 15 2015-02-01 13:31:03 PST

Comment on attachment 245825 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=245825&action=review r=me > Source/WebCore/history/BackForwardList.cpp:68 > - RefPtr<HistoryItem> item = m_entries.last(); > + auto& item = m_entries.last(); > m_entries.removeLast(); I'm a little iffy about using auto& in cases like this. Since 'item' may now be taking over the ownership of the HistoryItem, I'd prefer that we make that explicit in the code. Maybe I'm overly paranoid, but I'm also imagining a future where we'd do some kind of PeekType optimization for Vector and allow first(), last(), etc for Vector<RefPtr<T>> to just return T*. > Source/WebCore/history/BackForwardList.cpp:-80 > + auto& item = m_entries.first(); > m_entries.remove(0); > - m_entryHash.remove(item); Same comment. > Source/WebCore/history/BackForwardList.cpp:174 > - while (size < (int)m_entries.size()) { > - RefPtr<HistoryItem> item = m_entries.last(); > + while (size < static_cast<int>(m_entries.size())) { > + auto& item = m_entries.last(); Same comment. > Source/WebCore/history/BackForwardList.h:39 > -typedef Vector<RefPtr<HistoryItem>> HistoryItemVector; > +typedef Vector<Ref<HistoryItem>> HistoryItemVector; Nice :) Do you think we really need this typedef though?

Chris Dumez

Comment 16 2015-02-01 14:02:56 PST

Created attachment 245843 [details] Patch

WebKit Commit Bot

Comment 17 2015-02-01 14:05:19 PST

Attachment 245843 [details] did not pass style-queue: ERROR: Source/WebCore/history/HistoryItem.cpp:478: Multi line control clauses should use braces. [whitespace/braces] [4] ERROR: Source/WebCore/history/HistoryItem.cpp:498: Multi line control clauses should use braces. [whitespace/braces] [4] Total errors found: 2 in 25 files If any of these errors are false positives, please file a bug against check-webkit-style.

Chris Dumez

Comment 18 2015-02-01 15:28:32 PST

Created attachment 245848 [details] Patch

WebKit Commit Bot

Comment 19 2015-02-01 15:30:27 PST

Attachment 245848 [details] did not pass style-queue: ERROR: Source/WebCore/history/HistoryItem.cpp:478: Multi line control clauses should use braces. [whitespace/braces] [4] ERROR: Source/WebCore/history/HistoryItem.cpp:498: Multi line control clauses should use braces. [whitespace/braces] [4] Total errors found: 2 in 26 files If any of these errors are false positives, please file a bug against check-webkit-style.

WebKit Commit Bot

Comment 20 2015-02-01 16:49:33 PST

Comment on attachment 245848 [details] Patch Clearing flags on attachment: 245848 Committed r179472: <http://trac.webkit.org/changeset/179472>

WebKit Commit Bot

Comment 21 2015-02-01 16:49:37 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.