Summary: | Implement currentThreadStackBase on WinCE | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Joe Mason <joenotcharles> | ||||||||
Component: | Platform | Assignee: | Nobody <webkit-unassigned> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | manyoso, yong.li.webkit | ||||||||
Priority: | P2 | ||||||||||
Version: | 528+ (Nightly build) | ||||||||||
Hardware: | PC | ||||||||||
OS: | Other | ||||||||||
Bug Depends on: | |||||||||||
Bug Blocks: | 23154 | ||||||||||
Attachments: |
|
Description
Joe Mason
2009-06-22 10:56:21 PDT
Created attachment 31657 [details]
currentThreadStackBase for WinCE
This patch is from Yong Li.
Comment on attachment 31657 [details] currentThreadStackBase for WinCE > +2009-06-22 Yong Li <yong.li@torchmobile.com> > + > + Reviewed by NOBODY (OOPS!). > + > + https://bugs.webkit.org/show_bug.cgi?id=26611 > + Implement currentThreadStackBase on WINCE You should include the full description of the change here. As well as document each new function you are adding where it is non-obvious. Good to be concise, but not at the expense of true explanation. > +inline bool isPageWritable(void* p) Please do not abbreviate the names of variables. Spell out the variable using a good descriptive name. Err on code readability and maintenance instead of less typing. > + void* sp = (void*)(&lower); > + register char* curPage = (char*)((DWORD)sp & ~(pageSize - 1)); Same as above. Comment on attachment 31657 [details] currentThreadStackBase for WinCE > +2009-06-22 Yong Li <yong.li@torchmobile.com> > + > + Reviewed by NOBODY (OOPS!). > + > + https://bugs.webkit.org/show_bug.cgi?id=26611 > + Implement currentThreadStackBase on WINCE You should include the full description of the change here. As well as document each new function you are adding where it is non-obvious. Good to be concise, but not at the expense of true explanation. > +inline bool isPageWritable(void* p) Please do not abbreviate the names of variables. Spell out the variable using a good descriptive name. Err on code readability and maintenance instead of less typing. > + void* sp = (void*)(&lower); > + register char* curPage = (char*)((DWORD)sp & ~(pageSize - 1)); Same as above. From Adam: Also, the 'for (;;)' looks dangerous if 'isPageWritable' never returns false. We can fix this by checking that curPage > 0 when scanning downwards, or < some max bound when scanning upwards. Yong suggests 0x8000000. Created attachment 31682 [details]
updated patch
This should fix the above issues
Comment on attachment 31682 [details] updated patch Much better style, but still a few nits: > +inline bool isPageWritable(void* page) > +{ > + MEMORY_BASIC_INFORMATION buf; > + DWORD result = VirtualQuery(page, &buf, sizeof(buf)); s/buf/buffer/ > + if (!pageSize) { > + SYSTEM_INFO sysInfo; s/sysInfo/systemInfo/ > + register char* curPage = (char*)((DWORD)thisFrame & ~(pageSize - 1)); s/curPage/currentPage/ Also, Yong wrote some of this right? The ChangeLog should include all authors. Created attachment 31720 [details]
third patch
|