Bug 119063 - HistoryController: setting the pageScaleFactor and scrollPoint properly
Summary: HistoryController: setting the pageScaleFactor and scrollPoint properly
Status: RESOLVED DUPLICATE of bug 126022
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-07-24 13:02 PDT by Thiago de Barros Lacerda
Modified: 2013-12-19 16:06 PST (History)
16 users (show)

See Also:


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

Note You need to log in before you can comment on or make changes to this bug.
Description Thiago de Barros Lacerda 2013-07-24 13:02:04 PDT
HistoryController: setting the pageScaleFactor and scrollPoint properly
Comment 1 Thiago de Barros Lacerda 2013-07-24 13:16:07 PDT
Created attachment 207409 [details]
Patch
Comment 2 Jesus Sanchez-Palencia 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 ;) .
Comment 3 Brady Eidson 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.
Comment 4 Jesus Sanchez-Palencia 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...
Comment 5 Thiago de Barros Lacerda 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?
Comment 6 Thiago de Barros Lacerda 2013-07-25 07:24:13 PDT
Created attachment 207457 [details]
Patch
Comment 7 Brady Eidson 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?
Comment 8 Thiago de Barros Lacerda 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?
Comment 9 Brady Eidson 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?
Comment 10 Thiago de Barros Lacerda 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.
Comment 11 Thiago de Barros Lacerda 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
Comment 12 Thiago de Barros Lacerda 2013-09-05 13:41:53 PDT
Created attachment 210657 [details]
Patch
Comment 13 Thiago de Barros Lacerda 2013-09-05 14:21:50 PDT
Rebased on master
Comment 14 Darin Adler 2013-09-06 13:43:02 PDT
Comment on attachment 210657 [details]
Patch

Can you make a regression test for this, please?
Comment 15 Sam Weinig 2013-09-10 14:50:59 PDT
Comment on attachment 210657 [details]
Patch

As many have noted, this needs a test.
Comment 16 Thiago de Barros Lacerda 2013-12-19 16:06:32 PST

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