Bug 119063

Summary:

HistoryController: setting the pageScaleFactor and scrollPoint properly

Product:

WebKit

Reporter:

Thiago de Barros Lacerda <thiago.lacerda>

Component:

WebCore Misc.

Assignee:

Nobody <webkit-unassigned>

Status:

RESOLVED DUPLICATE

Severity:

Normal

CC:

bdakin, beidson, cmarcelo, commit-queue, hugo.lima, japhet, jesus, kenneth, lauro.neto, luciano.wolf, luiz, menard, noam, sergio, thorton, zalan

Priority:

Version:

528+ (Nightly build)

Hardware:

Unspecified

OS:

Unspecified

Attachments:

Description	Flags
Patch	none
Patch	none
Patch	sam: review-

Thiago de Barros Lacerda

Reported 2013-07-24 13:02:04 PDT

HistoryController: setting the pageScaleFactor and scrollPoint properly

Attachments
Patch (2.68 KB, patch) 2013-07-24 13:16 PDT, Thiago de Barros Lacerda	no flags	Details Formatted Diff Diff
Patch (2.68 KB, patch) 2013-07-25 07:24 PDT, Thiago de Barros Lacerda	no flags	Details Formatted Diff Diff
Patch (2.42 KB, patch) 2013-09-05 13:41 PDT, Thiago de Barros Lacerda	sam: review-	Details Formatted Diff Diff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.

Thiago de Barros Lacerda

Comment 1 2013-07-24 13:16:07 PDT

Created attachment 207409 [details] Patch

Jesus Sanchez-Palencia

Comment 2 2013-07-24 14:55:17 PDT

Comment on attachment 207409 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=207409&action=review > Source/WebCore/ChangeLog:8 > + No new tests (OOPS!). You should either remove the entire line or state "No new tests needed because of foo....". Leaving the (OOPS!) here is wrong ;) .

Brady Eidson

Comment 3 2013-07-24 15:50:22 PDT

(In reply to comment #2) > (From update of attachment 207409 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=207409&action=review > > > Source/WebCore/ChangeLog:8 > > + No new tests (OOPS!). > > You should either remove the entire line or state "No new tests needed because of foo....". Leaving the (OOPS!) here is wrong ;) . Correction - You should either add tests, explain what tests cover this already, or explain why tests are not possible. It's not okay to just remove this line without explanation.

Jesus Sanchez-Palencia

Comment 4 2013-07-24 21:25:03 PDT

(In reply to comment #3) > It's not okay to just remove this line without explanation. Well, sure, crystal clear everything I said is assuming if you left a line saying "NO NEW TESTS" you know what you are doing, therefore you are not adding new tests due to a reason. Needless to say adding tests is usually better...

Thiago de Barros Lacerda

Comment 5 2013-07-25 07:22:44 PDT

There are not tests for HistoryController, even with the old behaviour. Do you think it is worth adding test for that?

Thiago de Barros Lacerda

Comment 6 2013-07-25 07:24:13 PDT

Created attachment 207457 [details] Patch

Brady Eidson

Comment 7 2013-07-25 09:16:18 PDT

(In reply to comment #5) > There are not tests for HistoryController, even with the old behaviour. > Do you think it is worth adding test for that? Is it testable?

Thiago de Barros Lacerda

Comment 8 2013-07-25 09:25:12 PDT

(In reply to comment #7) > (In reply to comment #5) > > There are not tests for HistoryController, even with the old behaviour. > > Do you think it is worth adding test for that? > > Is it testable? I can think now of a simple test, creating a HistoryController instance and 2 HistoryItems, with different scales and scroll points, and play around going back and forward with them. I think it is enough for testing it. What do you think?

Brady Eidson

Comment 9 2013-07-25 09:31:10 PDT

(In reply to comment #8) > (In reply to comment #7) > > (In reply to comment #5) > > > There are not tests for HistoryController, even with the old behaviour. > > > Do you think it is worth adding test for that? > > > > Is it testable? > > I can think now of a simple test, creating a HistoryController instance and 2 HistoryItems, with different scales and scroll points, and play around going back and forward with them. I think it is enough for testing it. What do you think? How does that translate to a LayoutTest?

Thiago de Barros Lacerda

Comment 10 2013-07-31 10:27:44 PDT

(In reply to comment #9) > (In reply to comment #8) > > (In reply to comment #7) > > > (In reply to comment #5) > > > > There are not tests for HistoryController, even with the old behaviour. > > > > Do you think it is worth adding test for that? > > > > > > Is it testable? > > > > I can think now of a simple test, creating a HistoryController instance and 2 HistoryItems, with different scales and scroll points, and play around going back and forward with them. I think it is enough for testing it. What do you think? > > How does that translate to a LayoutTest? I have been trying for the last days to simulate the behaviour I'm facing with the UIProcess running, to a LayoutTest... But no success... the code paths are _very_ different.

Thiago de Barros Lacerda

Comment 11 2013-08-23 13:28:29 PDT

(In reply to comment #9) > (In reply to comment #8) > > (In reply to comment #7) > > > (In reply to comment #5) > > > > There are not tests for HistoryController, even with the old behaviour. > > > > Do you think it is worth adding test for that? > > > > > > Is it testable? > > > > I can think now of a simple test, creating a HistoryController instance and 2 HistoryItems, with different scales and scroll points, and play around going back and forward with them. I think it is enough for testing it. What do you think? > > How does that translate to a LayoutTest? Brady, I couldn't make a LayoutTest on that. But as you can see this patch is very reasonable and we need it in Nix port. Additionally I can't see how it can add any problems to other ports. Regards

Thiago de Barros Lacerda

Comment 12 2013-09-05 13:41:53 PDT

Created attachment 210657 [details] Patch

Thiago de Barros Lacerda

Comment 13 2013-09-05 14:21:50 PDT

Rebased on master

Darin Adler

Comment 14 2013-09-06 13:43:02 PDT

Comment on attachment 210657 [details] Patch Can you make a regression test for this, please?

Sam Weinig

Comment 15 2013-09-10 14:50:59 PDT

Comment on attachment 210657 [details] Patch As many have noted, this needs a test.

Thiago de Barros Lacerda

Comment 16 2013-12-19 16:06:32 PST

*** This bug has been marked as a duplicate of bug 126022 ***

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