Closed GoogleCodeExporter closed 9 years ago
attached diff
Original comment by creavenm...@gmail.com
on 13 Aug 2010 at 3:58
Attachments:
Thanks for this Anton.
I'm very excited about your changes, so I have indeed been working on importing
them for some time now.
However, there are a number of problems, the main one of which is that the
changes cover too many areas for me to simply import in one go. Even seemingly
small changes may have large consequences, and so it's very risky to import
them in an undifferentiated way.
For example, in your revision b38ea381 you have changed the code that detects
which browser we are running in. I'm sure the code is fine, but since that
could have a major impact on other code further down the line, it's a good idea
for this change to have its own revision entry in the main repo.
So I'm therefore going through each of your changes and grouping them together,
ready to commit as a set of patches. That way we don't get every single
revision in the main repo, but we do get more than a simple commit of all
changes.
As we've discussed, for the future it's important to submit patches that deal
with a discrete area so that we can more easily see what is being changed.
(This is not your fault -- we've been evolving how we manage revisions and
patches, and it's only through the experience of bringing in your code that
I've got a better idea of how to do it.)
Anyway, these changes are looking good, so please bear with me a little longer
as we try to get them incorporated.
Original comment by mark.bir...@gtempaccount.com
on 14 Aug 2010 at 9:05
I should add, by the way, that every time I try to apply one of your revisions
as a patch (or use 'hg transplant') it gets rejected. It seems that there is
something awry with the EOL characters, which causes the patch to fail to
synchronise with the original source. One possibility is that the although the
files are stored with CRLF (to help our Windows friends!), patches seem to be
applied based on just LF.
So that you can try to reproduce the problem, my set-up is as follows:
anton: an alias for a clone of 'creavenmoro-backplanejs'
backplanejs: a clone of the main 'backplanejs' repo
In the backplanejs directory I generate a patch from a revision in the 'anton'
repo with the following command:
hg export -R anton b38ea381f18fda98f80c5e10e1a86ad3d24e3a6f | hg qimport -n anton-sniffer -
If I then try to apply this patch with:
hg qpush
I get a single rejected hunk at line 14. If I view the .rej file I see the
patch that supposed to be applied but there are additional blank lines between
each line.
Original comment by mark.bir...@gtempaccount.com
on 15 Aug 2010 at 12:49
no blank lines for me, just lines from new and old files:
http://skitch.com/creaven/d1gwa/sniffer.js.rej
Original comment by creavenm...@gmail.com
on 16 Aug 2010 at 4:39
The import problem seems to be caused by your editor being set to use LF by
default, so when you committed code it appears you changed the files from CRLF
to LF.
This would have been fine if I was taking all of your changesets in order, but
since I was working through the changesets and picking bits from here and
there, every single patch I tried was being rejected, because the EOL
characters were always different.
The only way around this would have been to know which changeset caused the EOL
characters to be changed from CRLF to LF, and then apply that patch first, but
since there are many changesets that just tidy up the code, that would have
taken an age.
Anyway, I've now added the Mercurial EOL extension which allows us to configure
things so that whenever we extract a file from the repo it will use the EOL
character for the native system. Since we're on Macs this means we'll get
LF-only files. And since that matches the patches that I'm creating from your
repo, then I can now apply them.
Windows users will get CRLF files, but hopefully that will be ok, if they
aren't patching. We will have to decide what the default EOL format should be
for the repo itself, since we have a mixture of Windows and Mac users, but we
can revisit this later -- at least for now I'm finally making progress!
Original comment by mark.bir...@gtempaccount.com
on 1 Sep 2010 at 2:18
Hi Anton,
There's a problem with one set of changes, but I don't know what it is, so I'm
bouncing it back to you.
To see the issue in action, do the following:
1. Take a clone of my personal backplanejs repo which contains the three patch
branches that I am working on (i.e, your changesets):
hg clone https://markbirbeck-backplanejs.googlecode.com/hg/ markbirbeck-backplanejs
The default branch is a copy of the master repo. This build has decoration
switched off for FF, and it has a test for external instance loading.
2. Before looking at the patch branches verify that all of this is working ok:
ant compile && ant test-compile && ant functional-test
3. Move to the first patch branch in the sequence:
hg update anton-housekeeping
This contains various changes you've made to the formatting, so shouldn't
affect the code -- and indeed, at this point the tests all still pass correctly.
4. Move to the decorator refactoring patch branch:
hg update anton-decorator
Again, compile everything and run the tests, and this time you'll get failures.
In order to get a full report I had to increase the Selenium RC timeout value,
but that could be because my machine is running a bit slow. If you find you
need to do the same, the value is at line 33 of the selenium-rc.xml file, and I
set it as follows:
<attribute name="timeout" default="300" />
(I've changed this in the master repo.)
5. Restore the Firefox decoration support:
diff --git a/third-party/uxf/src/lib/sniffer.js
b/third-party/uxf/src/lib/sniffer.js
--- a/third-party/uxf/src/lib/sniffer.js
+++ b/third-party/uxf/src/lib/sniffer.js
@@ -38,7 +38,7 @@
UX.isXHTML = !!(document.xmlVersion || (document.contentType && document.contentType === "application/xhtml+xml"));
- UX.hasDecorationSupport = UX.isIE;
+ UX.hasDecorationSupport = UX.isIE || UX.isFF;
UX.isQuirksMode = document.compatMode === "BackCompat";
})();
You'll find that all tests now pass except for "Test loading external
instances".
RECAP
There are two problems after the refactoring:
* in FF, nothing works unless XBL mode is used;
* in all browsers, external instances can't be loaded.
I doubt that they are very big problems but I don't want to go any further with
the integration until they are fixed.
If you create a fresh online clone (or clear your current one) and then modify
the patch branches that I've created then I can just do a pull from you and
then carry on with my integration.
(I have a full copy of your repo so it shouldn't be a problem if you clear your
current one. Don't make any further changes to the default branch though, since
that should be used to keep in sync with the master repo.)
Thanks!
Original comment by mark.bir...@gtempaccount.com
on 1 Sep 2010 at 11:21
fix attached
Original comment by creavenm...@gmail.com
on 3 Sep 2010 at 11:17
Attachments:
Thanks Anton. The patch has been incorporated, and it fixes the
'anton-decorator' branch and the 'anton-namespace-manager' branch.
The 'anton-optimise-behavior' branch still doesn't work though. That was quite
a complex merge because Frankie had also changed some of the files that you had
updated, so I may have made a mistake. Anyway, it would be good if you could
take a look at that branch next.
Thanks.
Original comment by mark.bir...@gtempaccount.com
on 3 Sep 2010 at 12:00
Should also add that your patch fixes the 'anton-htc-fix' branch too.
Original comment by mark.bir...@gtempaccount.com
on 3 Sep 2010 at 12:12
optimise-behavior contains only one commit, it requires all other
Original comment by creavenm...@gmail.com
on 5 Sep 2010 at 8:34
I thought that might be the case and I've been working on bringing in other
changesets on top of that branch. It's a bit slow though because some of these
revisions include fixes and changes for a variety of things.
Anyway...progress is being made. :)
Original comment by mark.bir...@gmail.com
on 5 Sep 2010 at 9:25
Fixed in re436949ff2.
Original comment by mark.bir...@gtempaccount.com
on 23 Nov 2010 at 11:02
Original issue reported on code.google.com by
creavenm...@gmail.com
on 13 Aug 2010 at 2:50