WIP: snapshot support #70

annevk · 2018-05-15T12:22:25Z

This makes some progress on snapshots, but it's a lot of effort...

See whatwg/html-build#151 and whatwg/wattsi#70. We should probably change wattsi such that everything annotated w-nohtml does not always have to be annotated with w-nosnap...

annevk · 2018-05-15T12:28:02Z

On top of this we'd have to support yet another variant that only builds a single singlepage document with "Review Draft" relevant output. I'm not sure I want to put in the effort though since this took me quite a while and I'm getting nowhere fast.

annevk · 2018-05-15T12:28:11Z

cc @domenic

This makes some progress on snapshots, but it's a lot of effort...

annevk · 2018-05-15T16:33:23Z

This setup requires annotating most w-nohtml annotations in HTML also with w-nosnap. It would be nice to improve on that. I think we should introduce w-dev as a way of omitting things when Variant <> vDEV.

As for Review Drafts, my idea was that we would support a seventh argument to the wattsi invocation that would take care of that. Ideally as a modification on top of the vSnap variant.

This supports whatwg/wattsi#70.

annevk · 2018-05-16T08:45:29Z

src/wattsi.pas

+            Scratch := Default(Rope);
+            Scratch.Append('https://github.com/whatwg/html/commit/');
+            Scratch.Append(@SourceGitSHA[1], Length(SourceGitSHA));
+            Element.SetAttributeDestructively('href', Scratch);


This bit here doesn't work. End up with a bunch of wrong bytes in the final output. Elsewhere I could work around combining an AnsiString with some other type of string, but that doesn't work when setting attributes. I'd appreciate some help.

cc @Hixie @sideshowbarker

sideshowbarker · 2018-05-17T05:26:08Z

+            Element.SetAttributeDestructively('href', Scratch);

This bit here doesn't work. End up with a bunch of wrong bytes in the final output. Elsewhere I could work around combining an AnsiString with some other type of string, but that doesn't work when setting attributes.

0751161 is a workaround for now. It’s not the right way but it gets the job done as far as I can tell, and I figured it’s better to go ahead and push this to the branch for testing at least..

But if we want to have the snapshot stuff working sooner rather than later — and as long as the rest of it other than this is working — then it wouldn’t be harmful to go ahead and merge this to master as-is and plan a follow-up commit for later to do this the right way.

As far as what’s actually the right way: In working on previous patches, I’ve run into the same problem before when trying to deal with using the Rope stuff and with some of the related functions expecting constants instead of mutable variables. However right now I can’t remember exactly how I ended up dealing with it (or avoiding it) in those other cases. But I can probably (re)figure it out by tomorrow or so. Or else maybe if @Hixie can take a quick look it may be obvious to him what we should do instead.

But anyway as I said, I think 0751161 gets us what we need for now.

annevk · 2018-05-17T14:02:37Z

src/wattsi.pas

-                  {$IFDEF TIMINGS} Writeln('Elapsed time: ', MillisecondsBetween(StartTime, Now()), 'ms'); {$ENDIF}
-                  // output...
-                  if (Variant <> vDEV) then
+                  for Variant in TVariants do


There's some copying of documents earlier on that also does that for all variants. I suppose we could add additional branching there too to save a bit of a processing time.

domenic

This seems pretty solid to me, although I am mostly judging on results...

This reverts 0751161 in favor of using Rope.Append in roughly the same sort of way it’s used in similar cases in the source.

sideshowbarker · 2018-05-19T06:59:05Z

+            Scratch.Append('https://….com/whatwg/html/commit/');
+            Scratch.Append(@SourceGitSHA[1], Length(SourceGitSHA));
+            Element.SetAttributeDestructively('href', Scratch);

6d066f9 (re)fixes that — and unlike the previous hack I pushed in 0751161, 6d066f9 does it in the right way. Or it’s at least something closer to the right way (where “right” is basically just however @Hixie would have done it himself…).

I got there after spending time looking back at the Rope sources and remembering where I ran into problems with it myself before. And that’s specifically in the fact that if you give Scratch.Append(…) a string, it must be a “short” string — because if you don’t, it won’t work as expected.

As far as what doesn’t work as expected, I think usually you end up with unexpected bytes in the output (because I guess some overflow has happened and you start getting stuff from other memory addresses) — but I think sometimes it also just causes a fatal error and “Access violation” message.

Anyway, regardless, in any case it always leads to breakage at runtime without a clear error message about what the actual specific cause is.

What it ideally should do instead is, fail to compile to begin with — or if not that, at least generate a fatal error at runtime with a specific message saying, “String must be no longer than NN characters.“

As far as what the code considers “short”: It appears to be platform-dependent but it seems in my environment at least (and maybe in all 64-bit environments), it’s 11 characters or less.

So what you what you need to do instead is, assign the string to a variable and then give Scratch.Append(…) a pointer to that. Which is what 6d066f9 does.

annevk · 2018-05-22T08:29:23Z

src/wattsi.pas

-                  if (not Split(Documents[Variant], BigTOC, OutputDirectory + '/multipage-' + kSuffixes[Variant] + '/')) then
-                     raise EAbort.Create('Could not split specification');
-                  {$IFDEF TIMINGS} Writeln('Elapsed time: ', MillisecondsBetween(StartTime, Now()), 'ms'); {$ENDIF}
+                  Result := True;


Should this move down again?

annevk · 2018-05-22T08:45:18Z

Commit message:

Add support for Commit Snapshots and Review Drafts

This enables singlepage Commit Snapshots and Review Drafts with the appropriate annotations, while keeping most of the input for them in whatwg/html and whatwg/html-build.

This depends on https://github.com/whatwg/html-build/pull/151 and https://github.com/whatwg/html/pull/3689 landing around the same time.

annevk · 2018-05-22T09:00:47Z

I fixed my own two nits, although avoiding additional cloning comes at a bit of additional code.

sideshowbarker

Tested and reviewed and all LGTM

See whatwg/meta#92 for details. And whatwg/wattsi#70 and whatwg/html#3689 for changes that are needed as well.

See whatwg/meta#92 for details. And whatwg/html-build#151 and whatwg/wattsi#70 for changes that are needed as well.

annevk added a commit to whatwg/html-build that referenced this pull request May 15, 2018

WIP: snapshot support

76ccc88

This supports whatwg/wattsi#70.

annevk mentioned this pull request May 15, 2018

WIP: snapshot support whatwg/html-build#151

Merged

annevk added a commit to whatwg/html that referenced this pull request May 15, 2018

WIP: snapshot support

2fae5cd

See whatwg/html-build#151 and whatwg/wattsi#70. We should probably change wattsi such that everything annotated w-nohtml does not always have to be annotated with w-nosnap...

annevk mentioned this pull request May 15, 2018

WIP: snapshot support whatwg/html#3689

Merged

annevk added 2 commits May 15, 2018 16:41

WIP: snapshot support

3c74bd2

This makes some progress on snapshots, but it's a lot of effort...

fixup

0acb4ae

annevk force-pushed the annevk/snapshot branch from 9cddc64 to 0acb4ae Compare May 15, 2018 14:41

annevk added a commit to whatwg/html-build that referenced this pull request May 16, 2018

WIP: snapshot support

3840359

This supports whatwg/wattsi#70.

more modest progress

4924d1d

annevk commented May 16, 2018

View reviewed changes

Abuse typed constant to set sha-link attribute

0751161

add review functionality

d77afcf

annevk commented May 17, 2018

View reviewed changes

domenic approved these changes May 17, 2018

View reviewed changes

Use Rope.Append to set sha-link attribute

6d066f9

This reverts 0751161 in favor of using Rope.Append in roughly the same sort of way it’s used in similar cases in the source.

annevk commented May 22, 2018

View reviewed changes

annevk added 2 commits May 22, 2018 10:49

move Result := True down again

3705cb8

avoid too much document cloning

c974041

sideshowbarker reviewed May 22, 2018

View reviewed changes

annevk mentioned this pull request May 22, 2018

Deduplicate ProcessDocument/Save calls #72

Open

domenic mentioned this pull request May 22, 2018

Review draft and revamped snapshot work coordination thread whatwg/meta#92

Open

16 tasks

annevk merged commit b72cb56 into master May 29, 2018

annevk deleted the annevk/snapshot branch May 29, 2018 13:56

annevk added a commit to whatwg/html-build that referenced this pull request May 29, 2018

Add Commit Snapshot and Review Draft support

c38c237

See whatwg/meta#92 for details. And whatwg/wattsi#70 and whatwg/html#3689 for changes that are needed as well.

annevk added a commit to whatwg/html that referenced this pull request May 29, 2018

Meta: Commit Snapshot and Review Draft support

f9d325f

See whatwg/meta#92 for details. And whatwg/html-build#151 and whatwg/wattsi#70 for changes that are needed as well.

alice pushed a commit to alice/html that referenced this pull request Jan 8, 2019

Meta: Commit Snapshot and Review Draft support

687c49e

See whatwg/meta#92 for details. And whatwg/html-build#151 and whatwg/wattsi#70 for changes that are needed as well.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WIP: snapshot support #70

WIP: snapshot support #70

annevk commented May 15, 2018

annevk commented May 15, 2018

annevk commented May 15, 2018

annevk commented May 15, 2018

annevk May 16, 2018

sideshowbarker commented May 17, 2018 •

edited

Loading

annevk May 17, 2018

domenic left a comment

sideshowbarker commented May 19, 2018 •

edited

Loading

annevk May 22, 2018

annevk commented May 22, 2018

annevk commented May 22, 2018

sideshowbarker left a comment

WIP: snapshot support #70

WIP: snapshot support #70

Conversation

annevk commented May 15, 2018

annevk commented May 15, 2018

annevk commented May 15, 2018

annevk commented May 15, 2018

annevk May 16, 2018

Choose a reason for hiding this comment

sideshowbarker commented May 17, 2018 • edited Loading

annevk May 17, 2018

Choose a reason for hiding this comment

domenic left a comment

Choose a reason for hiding this comment

sideshowbarker commented May 19, 2018 • edited Loading

annevk May 22, 2018

Choose a reason for hiding this comment

annevk commented May 22, 2018

annevk commented May 22, 2018

sideshowbarker left a comment

Choose a reason for hiding this comment

sideshowbarker commented May 17, 2018 •

edited

Loading

sideshowbarker commented May 19, 2018 •

edited

Loading